Prepared by:
HALBORN
Last Updated 04/29/2025
Date of Engagement: April 3rd, 2025 - April 11th, 2025
100% of all REPORTED Findings have been addressed
All findings
14
Critical
0
High
3
Medium
5
Low
3
Informational
3
BSX
engaged Halborn to conduct a security assessment on their smart contracts beginning on April 7th, 2025 and ending on April 16th, 2025. The security assessment was scoped to smart contracts in the GitHub repository provided to the Halborn team. Commit hashes and further details can be found in the Scope section of this report.
The team at Halborn
assigned a full-time security engineer to assess the security of the smart contracts. The security engineer is a blockchain and smart-contract security expert with advanced penetration testing, smart-contract hacking, and deep knowledge of multiple blockchain protocols.
The purpose of this assessment is to:
Ensure that smart contract functions operate as intended.
Identify potential security issues with the smart contracts.
In summary, Halborn identified some improvements to reduce the likelihood and impact of risks, which were mostly addressed by the BSX team
. The main ones were the following:
Restrict the transfer of vault share tokens to prevent inconsistencies in the average price tracking.
Prevent the transfer of vault share tokens from subaccounts to main accounts during subaccount deletion.
Restrict subaccounts from performing swaps that result in the acquisition of vault shares.
Add an explicit check to ensure an account cannot be reassigned as a subaccount of itself.
Only finalize each operation's authorization once the transfer or withdrawal succeeds, so that failures don't permanently lock out retries.
Automatically close or liquidate all of the subaccount's open positions before deleting a subaccount.
Include a per‑main‑account nonce to track consumed nonces, so that any old consent signature cannot be replayed after revocation.
Record the token balance immediately before and after each ERC-4626 call, calculate the actual asset delta, and require that this observed change equals the returned output.
This assessment only covers the changes introduced between commit d1cad2e
and commit 2e8c754
(inclusive). All other parts of the codebase are treated as black boxes, and it is assumed that any required security mechanisms are properly implemented elsewhere in the system. This includes, but is not limited to, input validation, business logic enforcement, and access control.
Differential audits focus on identifying security risks introduced or modified in a specific set of changes. While some components added in the diff may interact with each other or with existing logic, these interactions are only reviewed when they are explicitly visible within the diff itself. Implicit assumptions or cross-component dependencies may fall outside the scope unless clearly surfaced through changes or supporting context. As an example, the feeAmount
field in swapYieldAssetPermit
was noted as unused during the review. The BSX team
confirmed this is expected and that future logic will handle it.
For a more complete understanding of the protocol's security posture and the behavior of interconnected components, a full-scope assessment is recommended.
Halborn
performed a combination of manual review of the code and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of the smart contract assessment. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of smart contracts and can quickly identify items that do not follow security best practices. The following phases and associated tools were used throughout the term of the assessment:
Research into the architecture, purpose, and use of the platform.
Smart contract manual code review and walkthrough to identify any logic issue.
Thorough assessment of safety and usage of critical Solidity variables and functions in scope that could lead to arithmetic related vulnerabilities.
Manual testing by custom scripts.
Graphing out functionality and contract logic/connectivity/functions (solgraph
).
Static Analysis of security for scoped contract, and imported functions. (Slither
).
Local or public testnet deployment (Foundry
, Remix IDE
).
EXPLOITABILITY METRIC () | METRIC VALUE | NUMERICAL VALUE |
---|---|---|
Attack Origin (AO) | Arbitrary (AO:A) Specific (AO:S) | 1 0.2 |
Attack Cost (AC) | Low (AC:L) Medium (AC:M) High (AC:H) | 1 0.67 0.33 |
Attack Complexity (AX) | Low (AX:L) Medium (AX:M) High (AX:H) | 1 0.67 0.33 |
IMPACT METRIC () | METRIC VALUE | NUMERICAL VALUE |
---|---|---|
Confidentiality (C) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
Integrity (I) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
Availability (A) | None (A:N) Low (A:L) Medium (A:M) High (A:H) Critical (A:C) | 0 0.25 0.5 0.75 1 |
Deposit (D) | None (D:N) Low (D:L) Medium (D:M) High (D:H) Critical (D:C) | 0 0.25 0.5 0.75 1 |
Yield (Y) | None (Y:N) Low (Y:L) Medium (Y:M) High (Y:H) Critical (Y:C) | 0 0.25 0.5 0.75 1 |
SEVERITY COEFFICIENT () | COEFFICIENT VALUE | NUMERICAL VALUE |
---|---|---|
Reversibility () | None (R:N) Partial (R:P) Full (R:F) | 1 0.5 0.25 |
Scope () | Changed (S:C) Unchanged (S:U) | 1.25 1 |
Severity | Score Value Range |
---|---|
Critical | 9 - 10 |
High | 7 - 8.9 |
Medium | 4.5 - 6.9 |
Low | 2 - 4.4 |
Informational | 0 - 1.9 |
Critical
0
High
3
Medium
5
Low
3
Informational
3
Security analysis | Risk level | Remediation Date |
---|---|---|
Average price desynchronizes after external vault share moves | High | Solved - 04/23/2025 |
Incorrect price tracking after asset consolidation from subaccount deletion | High | Solved - 04/28/2025 |
Unrestricted acquisition of vault shares by subaccounts during swaps | High | Solved - 04/28/2025 |
Account can be self-converted into a subaccount | Medium | Solved - 04/22/2025 |
Nonces consumed on failed operations prevent retries | Medium | Risk Accepted - 04/22/2025 |
Subaccount deletion can be indefinitely blocked by open positions | Medium | Risk Accepted - 04/22/2025 |
Revoked signers can be reauthorized with old consent signatures | Medium | Risk Accepted - 04/22/2025 |
Unverified vault swap outputs can lead to inconsistent asset accounting | Medium | Solved - 04/28/2025 |
Unsupported asset pair aborts swap execution | Low | Risk Accepted - 04/28/2025 |
Improper balance management due to missing token authorization checks | Low | Risk Accepted - 04/28/2025 |
Residual token approval could remain after deposits | Low | Solved - 04/28/2025 |
Unbounded supported‑token list loops may exhaust gas | Informational | Acknowledged - 04/22/2025 |
Suboptimal gas usage due to post-increment in loops | Informational | Acknowledged - 04/22/2025 |
Minor spelling inconsistencies in error documentation | Informational | Solved - 04/22/2025 |
//
The ClearingService contract tracks the average price of vault shares deposited by each account. However, when vault shares are transferred externally using BalanceLogic.transfer
, the internal average price (avgPrice
) is not updated accordingly. This leads to an inaccurate average price record for the account, potentially affecting future operations that rely on historical pricing data.
The transfer
function does not update avgPrice
when sending vault shares:
/// @notice Transfers token between 2 accounts in the exchange, allowing:
/// - main -> main
/// - sub -> sub (same main)
/// - sub -> main
/// - main -> sub
function transfer(
IExchange exchange,
mapping(address => IExchange.Account) storage accounts,
BalanceEngine calldata engine,
address signer,
IExchange.TransferParams calldata params
) external {
address token = params.token;
address from = params.from;
address to = params.to;
uint256 nonce = params.nonce;
int256 amount = params.amount.safeInt256();
bool isValid = _validateTransfer(accounts, from, to);
if (!isValid) {
revert Errors.Exchange_Transfer_NotAllowed(from, to);
}
int256 fromBalance = engine.spot.getBalance(token, from);
if (fromBalance < amount) {
revert Errors.Exchange_Transfer_InsufficientBalance(from, token, fromBalance, amount.safeUInt256());
}
bytes32 digest =
exchange.hashTypedDataV4(keccak256(abi.encode(TRANSFER_TYPEHASH, from, to, token, amount, nonce)));
if (!UNIVERSAL_SIG_VALIDATOR.isValidSig(signer, digest, params.signature)) {
revert Errors.Exchange_InvalidSignature(signer);
}
engine.clearingService.transfer(from, to, amount, token);
}
It is recommended to restrict the transfer of vault share tokens to prevent inconsistencies in the avgPrice
tracking.
SOLVED: The issue was proactively identified at an early stage by the BSX team and successfully addressed in the specified commit id.
//
When a subaccount is deleted, its assets, including vault shares, are transferred to the main account. However, the internal average price (avgPrice
) is not updated accordingly in AccountLogic.deleteSubaccount
. This results in inaccurate price tracking for the main account, potentially impacting future operations that depend on historical pricing data.
The avgPrice
is not updated when transferring vault shares from a subaccount to the main account during subaccount deletion:
function deleteSubaccount(
mapping(address => IExchange.Account) storage accounts,
Access access,
IExchange exchange,
IExchange.DeleteSubaccountParams memory params
) external {
address main = params.main;
address subaccount = params.subaccount;
// Check if the main/sub account are main accounts
if (accounts[main].accountType != IExchange.AccountType.Main) {
revert Errors.Exchange_InvalidAccountType(main);
}
if (accounts[subaccount].accountType != IExchange.AccountType.Subaccount) {
revert Errors.Exchange_InvalidAccountType(subaccount);
}
if (accounts[subaccount].state == IExchange.AccountState.Deleted) {
revert Errors.Exchange_Subaccount_Deleted(subaccount);
}
// Check if the main account is the same
if (main != accounts[subaccount].main) {
revert Errors.Exchange_Subaccount_MainAccountMismatch(main, accounts[subaccount].main);
}
// Transfer all assets and debt to main account
IClearingService clearingService = access.getClearingService();
ISpot spotEngine = access.getSpotEngine();
address[] memory supportedTokens = exchange.getSupportedTokenList();
for (uint256 i = 0; i < supportedTokens.length; i++) {
address token = supportedTokens[i];
address yieldAsset = clearingService.yieldAssets(token);
_transferSubToMain(clearingService, spotEngine, token, main, subaccount);
if (yieldAsset != address(0)) {
_transferSubToMain(clearingService, spotEngine, yieldAsset, main, subaccount);
}
}
// Check if the subaccount has no open position
IPerp perpEngine = access.getPerpEngine();
if (perpEngine.openPositions(subaccount) != 0) {
revert Errors.Exchange_Subaccount_HasOpenPosition(subaccount);
}
bytes32 hash = exchange.hashTypedDataV4(keccak256(abi.encode(DELETE_SUBACCOUNT_TYPEHASH, main, subaccount)));
if (!UNIVERSAL_SIG_VALIDATOR.isValidSig(main, hash, params.mainSignature)) {
revert Errors.Exchange_InvalidSignature(main);
}
IExchange.Account storage subaccountData = accounts[subaccount];
subaccountData.state = IExchange.AccountState.Deleted;
IExchange.Account storage mainAccountData = accounts[main];
uint256 len = mainAccountData.subaccounts.length;
for (uint256 i = 0; i < len; i++) {
if (mainAccountData.subaccounts[i] == subaccount) {
mainAccountData.subaccounts[i] = mainAccountData.subaccounts[len - 1];
mainAccountData.subaccounts.pop();
break;
}
}
}
It is recommended to prevent the transfer of vault share tokens from subaccounts to main accounts during subaccount deletion to avoid inconsistencies in the main account's avgPrice
tracking.
//
The swapCollateralBatch
function in SwapLogic library allows subaccounts to acquire vault shares, either through swapYieldAssetPermit
or through a standard swap via innerSwapWithPermit
(in the latter case, if the share token is registered as supported). This leads to two major consequences:
Vault shares obtained by subaccounts can be transferred or inherited by the main account upon subaccount deletion, causing avgPrice
distortions similar to the previously reported vulnerabilities, see [HAL-01] Average price desynchronizes after external vault share moves
and [HAL-02] Incorrect price tracking after asset consolidation from subaccount deletion
.
Even if vault share transfers are restricted, deleting a subaccount would leave the vault shares trapped in the deleted subaccount, effectively locking the assets without a recovery path.
A Proof of Concept (PoC) was developed to demonstrate that a subaccount can successfully acquire vault shares by calling swapYieldAssetPermit
. The subaccount deposits tokens, executes the swap, and receives vault shares, confirming that there are no restrictions preventing subaccounts from obtaining vault assets. Below is the PoC:
function test_swapYieldAssetPermit_Subaccount() public {
IExchange.Account memory userAccount = exchange.accounts(user);
assertEq(uint8(userAccount.accountType), uint8(IExchange.AccountType.Main));
(address main, uint256 mainKey) = makeAddrAndKey("COFFEE");
bytes32 CREATE_SUBACCOUNT_TYPEHASH = keccak256("CreateSubaccount(address main,address subaccount)");
bytes32 structHash = keccak256(abi.encode(CREATE_SUBACCOUNT_TYPEHASH, main, user));
bytes memory mainSignature = _signTypedDataHash(mainKey, structHash);
bytes memory subSignature = _signTypedDataHash(userKey, structHash);
vm.prank(admin);
exchange.createSubaccount(main, user, mainSignature, subSignature);
userAccount = exchange.accounts(user);
assertEq(uint8(userAccount.accountType), uint8(IExchange.AccountType.Subaccount));
vm.prank(admin);
clearingService.addYieldAsset(token, vault);
// 1 share = 2 tokens
ERC20Simple(token).mint(vault, 1);
uint256 userBalance = 500e18;
_depositSpotAccount(user, userBalance, token);
uint256 nonce = 1;
uint256 depositAssets = 300e18;
uint256 mintShares = 150e18;
ISwap.SwapParams memory params;
params.account = user;
params.assetIn = token;
params.amountIn = depositAssets;
params.assetOut = vault;
params.minAmountOut = mintShares;
params.nonce = nonce;
params.signature = _signTypedDataHash(
userKey,
keccak256(
abi.encode(
SWAP_TYPEHASH,
user,
params.assetIn,
params.amountIn,
params.assetOut,
params.minAmountOut,
params.nonce
)
)
);
vm.prank(address(exchange));
clearingService.swapYieldAssetPermit(params);
clearingService.vaultShares(user, vault);
assertEq(spotEngine.getBalance(vault, user), int256(mintShares));
}
The execution of this PoC was successful:
It is recommended to restrict subaccounts from performing swaps that result in the acquisition of vault shares, to prevent average price inconsistencies and the risk of asset lockup during subaccount deletion.
//
The createSubaccount
function in the AccountLogic library (invoked by Exchange.createSubaccount
) does not prevent the caller from passing the same address for both main
and subaccount
. Although only callers with GENERAL_ROLE
can trigger this function, the check for main != subaccount
is missing. If an authorized actor mistakenly or intentionally supplies the same address for both inputs, the function executes successfully and overwrites the account record, turning the main account into a subaccount of itself and leaving its main slot pointing to itself. This causes any future actions that depend on the account being a valid main to fail.
Lack of validation allows a main account to become its own subaccount, leading to self-referencing state and breaking future logic that relies on a valid main-subaccount relationship:
IExchange.Account storage mainAccountData = accounts[main];
mainAccountData.subaccounts.push(subaccount);
IExchange.Account storage subaccountData = accounts[subaccount];
subaccountData.main = main;
subaccountData.accountType = IExchange.AccountType.Subaccount;
It is recommended to add an explicit require(main != subaccount)
check in AccountLogic.createSubaccount
to ensure an account cannot be reassigned as a subaccount of itself, which would otherwise lock its main account functionality.
SOLVED: The BSX team solved the issue in the specified commit id.
//
The _withdraw
, _transfer
and _transferToBSX1000
functions in the Exchange contract mark their authorization nonces as used before calling into BalanceLogic. If the subsequent call reverts due to insufficient funds or other errors, the function catches the failure and emits an event, but the nonce remains consumed. The same operation cannot be retried under the original approval, creating a permanent denial of service for that transaction.
The same situation happens in the swapCollateralBatch
function in the SwapLogic library.
Authorization nonces are marked used, so if the operation fails the nonce is still consumed and cannot be retried:
function _withdraw(Withdraw memory data) internal enabledWithdraw {
if (isWithdrawNonceUsed[data.sender][data.nonce]) {
revert Errors.Exchange_Withdraw_NonceUsed(data.sender, data.nonce);
}
isWithdrawNonceUsed[data.sender][data.nonce] = true;
try BalanceLogic.withdraw(
_accounts, _collectedFee, this, BalanceLogic.BalanceEngine(clearingService, spotEngine), data
) {
emit WithdrawSucceeded(data.token, data.sender, data.nonce, data.amount, 0, data.withdrawalSequencerFee);
} catch {
emit WithdrawFailed(data.sender, data.nonce, 0, 0);
}
}
/// @dev Handles a transfer between 2 accounts in the exchange
function _transfer(TransferParams memory params) internal {
address signer = _accounts[params.from].accountType == IExchange.AccountType.Subaccount
? _accounts[params.from].main
: params.from;
if (_isNonceUsed[signer][params.nonce]) {
revert Errors.Exchange_NonceUsed(signer, params.nonce);
}
_isNonceUsed[signer][params.nonce] = true;
try BalanceLogic.transfer(
this, _accounts, BalanceLogic.BalanceEngine(clearingService, spotEngine), signer, params
) {
emit Transfer(
params.token,
params.from,
params.to,
signer,
params.nonce,
params.amount.safeInt256(),
ActionStatus.Success
);
} catch {
emit Transfer(
params.token,
params.from,
params.to,
address(0),
params.nonce,
params.amount.safeInt256(),
ActionStatus.Failure
);
}
}
function _transferToBSX1000(TransferToBSX1000Params memory params) internal {
if (isTransferToBSX1000NonceUsed[params.account][params.nonce]) {
revert Errors.Exchange_TransferToBSX1000_NonceUsed(params.account, params.nonce);
}
isTransferToBSX1000NonceUsed[params.account][params.nonce] = true;
try BalanceLogic.transferToBSX1000(
_accounts,
this,
IBSX1000x(access.getBsx1000()),
BalanceLogic.BalanceEngine(clearingService, spotEngine),
params
) returns (uint256 balance) {
emit TransferToBSX1000(
params.token, params.account, params.nonce, params.amount, balance, TransferToBSX1000Status.Success
);
} catch {
emit TransferToBSX1000(
params.token, params.account, params.nonce, params.amount, 0, TransferToBSX1000Status.Failure
);
}
}
function swapCollateralBatch(
mapping(address => mapping(uint256 => bool)) storage isSwapNonceUsed,
Exchange exchange,
ISwap.SwapParams[] calldata params
) external {
for (uint256 i = 0; i < params.length; i++) {
if (exchange.isSwapNonceUsed(params[i].account, params[i].nonce)) {
revert Errors.Exchange_Swap_NonceUsed(params[i].account, params[i].nonce);
}
isSwapNonceUsed[params[i].account][params[i].nonce] = true;
if (params[i].commands.length == 1 && params[i].commands[0] == Commands.MORPHO_PULL_ASSET) {
exchange.clearingService().swapYieldAssetPermit(params[i]);
} else {
try exchange.innerSwapWithPermit(params[i]) returns (uint256 amountOutX18) {
emit ISwap.SwapCollateral(
params[i].account,
params[i].nonce,
params[i].assetIn,
params[i].amountIn,
params[i].assetOut,
amountOutX18,
params[i].assetIn,
params[i].feeAmount,
ISwap.SwapCollateralStatus.Success
);
} catch {
emit ISwap.SwapCollateral(
params[i].account,
params[i].nonce,
params[i].assetIn,
params[i].amountIn,
params[i].assetOut,
0,
params[i].assetIn,
0,
ISwap.SwapCollateralStatus.Failure
);
}
}
}
}
It is recommended to update the authorization nonce only after the corresponding operation completes successfully. Nonces should not be marked as used in cases where the operation fails, as this prevents legitimate retries using the same approval and may lead to unnecessary user friction or loss of functionality.
RISK ACCEPTED: The BSX team accepted the risk of this finding, and stated the following:
This behavior is intentional. Our system stores events using a primary key composed of
account + nonce
, so if there are two events (1 failed and 1 successful), it would create a conflict in our database.
//
The deleteSubaccount
function in the AccountLogic library reverts whenever the target subaccount has any open positions, allowing a malicious or non‑cooperative subaccount to indefinitely prevent its own deletion by keeping even a minimal position open. Because only the sequencer can close positions and only the subaccount (or its authorized signer) can submit matching orders, the main account has no direct way to force a closure. In practice this means a single "1 wei" position opened by the subaccount will lock the deleteSubaccount
call in a constant revert loop, consuming gas in each attempt and leaving orphaned subaccounts that clutter on‑chain state and complicate account management.
Deletion is blocked if the subaccount holds any open positions, causing an immediate revert:
// Check if the subaccount has no open position
IPerp perpEngine = access.getPerpEngine();
if (perpEngine.openPositions(subaccount) != 0) {
revert Errors.Exchange_Subaccount_HasOpenPosition(subaccount);
}
It is recommended to automatically close or liquidate all of the subaccount's open positions at the start of deleteSubaccount
before checking openPositions
, for example by calling modifyAccount
with zero balances for each product, so that no live position can block the removal.
RISK ACCEPTED: The BSX team accepted the risk of this finding and stated the following:
We’re aware of this issue. However, since signer revocation is not yet supported and we don't want to disrupt the current flow, we've decided to accept this risk for now.
//
In the SignerLogic library, the secondary approval step uses an EIP‑712 type called SignKey
(defined by the SIGN_KEY_TYPEHASH
constant) to capture a signer's consent. That hash is computed only over the fixed payload (address account)
and never changes, so once a signature is issued it remains valid forever. Even after a signer has been revoked via unregisterSigningWallet
, replaying the old SignKey
signature in registerSigner
will restore its privileges without fresh consent.
The registerSigner
function omits nonce or expiry, enabling indefinite signature reuse:
function registerSigner(
mapping(address => mapping(uint64 => bool)) storage isRegisterSignerNonceUsed,
mapping(address => mapping(address => bool)) storage signingWallets,
IExchange exchange,
IExchange.AddSigningWallet memory data
) external {
address sender = data.sender;
address signer = data.signer;
uint64 nonce = data.nonce;
if (isRegisterSignerNonceUsed[sender][nonce]) {
revert Errors.Exchange_AddSigningWallet_UsedNonce(sender, nonce);
}
isRegisterSignerNonceUsed[sender][nonce] = true;
// verify signature of sender
bytes32 registerHash = exchange.hashTypedDataV4(
keccak256(abi.encode(REGISTER_TYPEHASH, signer, keccak256(abi.encodePacked(data.message)), nonce))
);
if (!UNIVERSAL_SIG_VALIDATOR.isValidSig(sender, registerHash, data.walletSignature)) {
revert Errors.Exchange_InvalidSignature(sender);
}
// verify signature of authorized signer
bytes32 signKeyHash = exchange.hashTypedDataV4(keccak256(abi.encode(SIGN_KEY_TYPEHASH, sender)));
GenericLogic.verifySignature(signer, signKeyHash, data.signerSignature);
signingWallets[sender][signer] = true;
emit IExchange.RegisterSigner(sender, signer, nonce);
}
It is recommended to include a unique per‑registration nonce in the SignKey
payload and to track consumed nonces so that any old consent signature cannot be replayed after revocation.
RISK ACCEPTED: The BSX team accepted the risk of this finding and stated the following:
This is by design to prevent users from accidentally deleting accounts with open positions. Additionally, the main account can close orders on behalf of subaccount. On the backend side, we also support freezing subaccounts to prevent them from opening new positions.
//
The private function _callToErc4626Vault
in the ClearingService contract executes ERC-4626 vault deposits and redeems without verifying actual token movements. In the deposit path it calls vault.deposit(...)
on behalf of the service, and in the redeem path it calls vault.redeem(...)
sending assets back to the Exchange. Because neither branch measures balances before and after the vault call, a malicious or misbehaving vault could return a false amountOut
and cause the contract's internal accounting to diverge from on-chain balances.
Even if a vault address was previously approved via yieldAssets
, it remains an external contract that could be compromised or behave maliciously and return an incorrect amountOut
. From a security perspective, it is important to take a defensive approach when interacting with external contracts, validating outputs where feasible and assuming that they could behave in unexpected or non-compliant ways, even if they were trusted at the time of integration.
ERC-4626 vault deposits and redeems do not verify actual token movements:
if (swapType == SwapType.DepositVault) {
uint256 assets = amountIn.convertFromScale(token);
access.getExchange().requestToken(token, assets);
IERC20(token).forceApprove(vault, assets);
tokenOut = vault;
amountOut = IERC4626(vault).deposit(assets, address(this));
} else if (swapType == SwapType.RedeemVault) {
uint256 shares = amountIn.convertFromScale(vault);
tokenOut = token;
amountOut = IERC4626(vault).redeem(shares, address(access.getExchange()), address(this));
}
It is recommended to record the token balance immediately before and after each ERC-4626 call, calculate the actual asset delta, and require that this observed change equals the returned amountOut
. If they do not match, the transaction should revert so that internal accounting always aligns with real token transfers.
SOLVED: The BSX team solved the issue in the specified commit id.
//
The swapYieldAssetPermit
function in the ClearingService contract is intended to always report success or failure via SwapAssets
events without halting execution. Instead, its initial validation of assetIn
and assetOut
reverts the entire call when the assets are not paired in yieldAssets
, breaking the uniform event-driven error reporting design and preventing downstream batch processes from continuing.
The assetIn
/ assetOut
mapping check reverts on invalid swaps:
/// @inheritdoc IClearingService
function swapYieldAssetPermit(ISwap.SwapParams calldata params) external onlySequencer {
address account = params.account;
address assetIn = params.assetIn;
uint256 amountIn = params.amountIn;
address assetOut = params.assetOut;
uint256 minAmountOut = params.minAmountOut;
uint256 nonce = params.nonce;
SwapType swapType;
if (yieldAssets[assetIn] == assetOut) {
swapType = SwapType.DepositVault;
} else if (yieldAssets[assetOut] == assetIn) {
swapType = SwapType.RedeemVault;
} else {
revert Errors.ClearingService_InvalidSwap(assetIn, assetOut);
}
It is recommended to replace the direct revert on unsupported asset pairs with a SwapAssets
event indicating failure and then return, ensuring that invalid combinations do not abort execution and are consistently reported via events.
RISK ACCEPTED: The BSX team accepted this risk and stated the following:
This is intentional. If a transaction fails, we will notice and fix it immediately. It should indicate something wrong on our side.
//
The transfer
function in the BalanceLogic library and its caller _transfer
in the Exchange contract invoke clearingService.transfer
without checking that the token is in the supportedTokens
list. As a result Exchange can update Spot balances for any token address even if that token is not officially recognised. An operator can inflate user balances for unsupported tokens, leading to unauthorized internal state changes.
The transfer
function does not check whether the token is included in the supportedTokens
list:
function transfer(
IExchange exchange,
mapping(address => IExchange.Account) storage accounts,
BalanceEngine calldata engine,
address signer,
IExchange.TransferParams calldata params
) external {
address token = params.token;
address from = params.from;
address to = params.to;
uint256 nonce = params.nonce;
int256 amount = params.amount.safeInt256();
bool isValid = _validateTransfer(accounts, from, to);
if (!isValid) {
revert Errors.Exchange_Transfer_NotAllowed(from, to);
}
int256 fromBalance = engine.spot.getBalance(token, from);
if (fromBalance < amount) {
revert Errors.Exchange_Transfer_InsufficientBalance(from, token, fromBalance, amount.safeUInt256());
}
bytes32 digest =
exchange.hashTypedDataV4(keccak256(abi.encode(TRANSFER_TYPEHASH, from, to, token, amount, nonce)));
if (!UNIVERSAL_SIG_VALIDATOR.isValidSig(signer, digest, params.signature)) {
revert Errors.Exchange_InvalidSignature(signer);
}
engine.clearingService.transfer(from, to, amount, token);
}
It is recommended to verify that the token argument is contained in the supportedTokens
before calling clearingService.transfer
in BalanceLogic.transfer
and before any other spot balance update in the Exchange contract.
RISK ACCEPTED: The BSX team accepted the risk of this finding and stated the following:
We acknowledge this. Spot is designed for internal purposes, so we assume that the data sent to Spot is trusted.
//
The private function _callToErc4626Vault
in ClearingService approves the vault to spend a calculated amount of tokens and then calls IERC4626(vault).deposit(...)
without resetting the approval. If a vault contract is non-compliant or malicious it may consume less than the full allowance and retain a residual approval that could be used to transfer tokens unexpectedly.
Similarly, the transferToBSX1000
function in the BalanceLogic library approves the BSX1000x contract without resetting the allowance. Although BSX1000x is intended to be a trusted internal contract, resetting allowances consistently would reduce attack surface and align with best security practices.
The vault allowance is never reset after calling IERC4626(vault).deposit
:
function _callToErc4626Vault(address vault, address token, uint256 amountIn, SwapType swapType)
private
returns (uint256 amountOut)
{
if (token != IERC4626(vault).asset()) {
revert Errors.ClearingService_YieldAsset_AssetMismatch(token, vault);
}
address tokenOut;
if (swapType == SwapType.DepositVault) {
uint256 assets = amountIn.convertFromScale(token);
access.getExchange().requestToken(token, assets);
IERC20(token).forceApprove(vault, assets);
tokenOut = vault;
amountOut = IERC4626(vault).deposit(assets, address(this));
} else if (swapType == SwapType.RedeemVault) {
uint256 shares = amountIn.convertFromScale(vault);
tokenOut = token;
amountOut = IERC4626(vault).redeem(shares, address(access.getExchange()), address(this));
}
return amountOut.convertToScale(tokenOut);
}
The allowance is never reset after calling bsx1000.deposit
:
function transferToBSX1000(
mapping(address => IExchange.Account) storage accounts,
IExchange exchange,
IBSX1000x bsx1000,
BalanceEngine calldata engine,
IExchange.TransferToBSX1000Params calldata params
) external returns (uint256 newBalance) {
_assertMainAccount(accounts, params.account);
bytes32 digest = exchange.hashTypedDataV4(
keccak256(
abi.encode(TRANSFER_TO_BSX1000_TYPEHASH, params.account, params.token, params.amount, params.nonce)
)
);
if (!UNIVERSAL_SIG_VALIDATOR.isValidSig(params.account, digest, params.signature)) {
revert Errors.Exchange_InvalidSignature(params.account);
}
IERC20 collateralBSX1000 = bsx1000.collateralToken();
if (params.token != address(collateralBSX1000)) {
revert Errors.Exchange_TransferToBSX1000_InvalidToken(params.token, address(collateralBSX1000));
}
uint256 amountToTransfer = params.amount.convertFromScale(params.token);
if (amountToTransfer == 0) {
revert Errors.Exchange_ZeroAmount();
}
int256 currentBalance = engine.spot.getBalance(params.token, params.account);
if (currentBalance < params.amount.safeInt256()) {
revert Errors.Exchange_TransferToBSX1000_InsufficientBalance(params.account, currentBalance, params.amount);
}
engine.clearingService.withdraw(params.account, params.amount, params.token);
IERC20(params.token).forceApprove(address(bsx1000), amountToTransfer);
bsx1000.deposit(params.account, params.amount);
newBalance = currentBalance.safeUInt256() - params.amount;
}
It is recommended to reset the token approval back to zero immediately after the deposit call so that no leftover allowance remains.
SOLVED: The BSX team solved the issue in the specified commit id.
//
Both the AccountLogic library (in its subaccount creation and deletion flows) and the AdminLogic library (in its sequencer fee claiming flow) iterate over the entire array returned by Exchange.getSupportedTokenList()
. In AccountLogic's createSubaccount
and deleteSubaccount
, each token is checked or transferred unconditionally, and in AdminLogic's claimSequencerFees
, each supported token is processed in a similar unbounded loop. Although only callers with GENERAL_ROLE
can grow the token list or invoke these functions, an unchecked increase in registered tokens could push any of these loops beyond the block's gas limit and cause transactions to revert.
address[] memory supportedTokens = exchange.getSupportedTokenList();
for (uint256 i = 0; i < supportedTokens.length; i++) {
address token = supportedTokens[i];
// per‑token logic...
}
It is recommended to enforce a maximum on the length of the supported‑token array or to refactor these loops to process tokens in manageable batches or with explicit gas checks so that subaccount actions and fee claims cannot be accidentally blocked by gas exhaustion.
ACKNOWLEDGED: The BSX team acknowledged this issue and stated the following:
The supported list rarely changes and is relatively short, so this issue is considered negligible.
//
In multiple functions across the codebase, the for
loops use i++
(post-increment) for incrementing the loop counter. In Solidity, post-increment (i++
) is slightly less efficient than pre-increment (++i
) because i++
requires storing the original value of i
in a temporary variable before incrementing, which consumes more gas. Although the gas difference is minimal, especially in recent Solidity versions, it becomes noticeable in larger loops or frequently executed functions, leading to inefficiencies in contract execution. The affected functions are the following:
AccountLogic.createSubaccount
AccountLogic.deleteSubaccount
To optimize gas usage, especially when iterating over large arrays or loops, it is recommendd to replace i++
with ++i
. Pre-increment (++i
) does not require storing the old value of i
, making it slightly more efficient in terms of gas consumption.
ACKNOWLEDGED: The BSX team acknowledged this issue and stated the following:
Since we compile with
viaIR flag enabled
, there’s no difference between i++ and ++i.
//
The Errors library contains minor spelling mistakes that could reduce clarity or harm perceived code quality. Specifically, the word “depositting” is incorrectly spelled with a double “t” in the comment for Exchange_NotCollateralToken
. Additionally, there is a duplicated prefix in the comment for Exchange_Subaccount_IsMainAccount
where “ThroThrown” appears instead of the correct term “Thrown”. These are only documentation issues but should be addressed to maintain consistency and professional standards.
It is recommended to correct the spelling of “depositting” to “depositing” and fix the duplicated word “ThroThrown” to “Thrown” in the corresponding comments in the Errors library.
SOLVED: The BSX team solved the issue in the specified commit id.
Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.
// Download the full report
Subaccount & Yield
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed