Subaccount & Yield - BSX Exchange


Prepared by:

Halborn Logo

HALBORN

Last Updated 04/29/2025

Date of Engagement: April 3rd, 2025 - April 11th, 2025

Summary

100% of all REPORTED Findings have been addressed

All findings

14

Critical

0

High

3

Medium

5

Low

3

Informational

3


1. INTRODUCTION

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.


2. ASSESSMENT SUMMARY

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.


3. CAVEATS

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.


4. TEST APPROACH AND METHODOLOGY

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).


5. RISK METHODOLOGY

Every vulnerability and issue observed by Halborn is ranked based on two sets of Metrics and a Severity Coefficient. This system is inspired by the industry standard Common Vulnerability Scoring System.
The two Metric sets are: Exploitability and Impact. Exploitability captures the ease and technical means by which vulnerabilities can be exploited and Impact describes the consequences of a successful exploit.
The Severity Coefficients is designed to further refine the accuracy of the ranking with two factors: Reversibility and Scope. These capture the impact of the vulnerability on the environment as well as the number of users and smart contracts affected.
The final score is a value between 0-10 rounded up to 1 decimal place and 10 corresponding to the highest security risk. This provides an objective and accurate rating of the severity of security vulnerabilities in smart contracts.
The system is designed to assist in identifying and prioritizing vulnerabilities based on their level of risk to address the most critical issues in a timely manner.

5.1 EXPLOITABILITY

Attack Origin (AO):
Captures whether the attack requires compromising a specific account.
Attack Cost (AC):
Captures the cost of exploiting the vulnerability incurred by the attacker relative to sending a single transaction on the relevant blockchain. Includes but is not limited to financial and computational cost.
Attack Complexity (AX):
Describes the conditions beyond the attacker’s control that must exist in order to exploit the vulnerability. Includes but is not limited to macro situation, available third-party liquidity and regulatory challenges.
Metrics:
EXPLOITABILITY METRIC (mem_e)METRIC VALUENUMERICAL 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
Exploitability EE is calculated using the following formula:

E=meE = \prod m_e

5.2 IMPACT

Confidentiality (C):
Measures the impact to the confidentiality of the information resources managed by the contract due to a successfully exploited vulnerability. Confidentiality refers to limiting access to authorized users only.
Integrity (I):
Measures the impact to integrity of a successfully exploited vulnerability. Integrity refers to the trustworthiness and veracity of data stored and/or processed on-chain. Integrity impact directly affecting Deposit or Yield records is excluded.
Availability (A):
Measures the impact to the availability of the impacted component resulting from a successfully exploited vulnerability. This metric refers to smart contract features and functionality, not state. Availability impact directly affecting Deposit or Yield is excluded.
Deposit (D):
Measures the impact to the deposits made to the contract by either users or owners.
Yield (Y):
Measures the impact to the yield generated by the contract for either users or owners.
Metrics:
IMPACT METRIC (mIm_I)METRIC VALUENUMERICAL 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
Impact II is calculated using the following formula:

I=max(mI)+mImax(mI)4I = max(m_I) + \frac{\sum{m_I} - max(m_I)}{4}

5.3 SEVERITY COEFFICIENT

Reversibility (R):
Describes the share of the exploited vulnerability effects that can be reversed. For upgradeable contracts, assume the contract private key is available.
Scope (S):
Captures whether a vulnerability in one vulnerable contract impacts resources in other contracts.
Metrics:
SEVERITY COEFFICIENT (CC)COEFFICIENT VALUENUMERICAL VALUE
Reversibility (rr)None (R:N)
Partial (R:P)
Full (R:F)
1
0.5
0.25
Scope (ss)Changed (S:C)
Unchanged (S:U)
1.25
1
Severity Coefficient CC is obtained by the following product:

C=rsC = rs

The Vulnerability Severity Score SS is obtained by:

S=min(10,EIC10)S = min(10, EIC * 10)

The score is rounded up to 1 decimal places.
SeverityScore Value Range
Critical9 - 10
High7 - 8.9
Medium4.5 - 6.9
Low2 - 4.4
Informational0 - 1.9

6. SCOPE

Files and Repository
(a) Repository: contracts-core
(b) Assessed Commit ID: 2e8c754
(c) Items in scope:
  • contracts/1000x/BSX1000x.sol
  • contracts/exchange/lib/logic/AccountLogic.sol
  • contracts/exchange/lib/logic/AdminLogic.sol
↓ Expand ↓
Out-of-Scope: Third party dependencies and economic attacks. Furthermore, this assessment was limited to the differential changes between commit d1cad2e and commit 2e8c754. Components outside of the reviewed diff, pre-existing logic not modified by the changes, system-wide interactions not directly surfaced in the diff context, and external assumptions regarding input validation, business rules, and access controls were explicitly out of scope.
Remediation Commit ID:
Out-of-Scope: New features/implementations after the remediation commit IDs.

7. Assessment Summary & Findings Overview

Critical

0

High

3

Medium

5

Low

3

Informational

3

Security analysisRisk levelRemediation Date
Average price desynchronizes after external vault share movesHighSolved - 04/23/2025
Incorrect price tracking after asset consolidation from subaccount deletionHighSolved - 04/28/2025
Unrestricted acquisition of vault shares by subaccounts during swapsHighSolved - 04/28/2025
Account can be self-converted into a subaccountMediumSolved - 04/22/2025
Nonces consumed on failed operations prevent retriesMediumRisk Accepted - 04/22/2025
Subaccount deletion can be indefinitely blocked by open positionsMediumRisk Accepted - 04/22/2025
Revoked signers can be reauthorized with old consent signaturesMediumRisk Accepted - 04/22/2025
Unverified vault swap outputs can lead to inconsistent asset accountingMediumSolved - 04/28/2025
Unsupported asset pair aborts swap executionLowRisk Accepted - 04/28/2025
Improper balance management due to missing token authorization checksLowRisk Accepted - 04/28/2025
Residual token approval could remain after depositsLowSolved - 04/28/2025
Unbounded supported‑token list loops may exhaust gasInformationalAcknowledged - 04/22/2025
Suboptimal gas usage due to post-increment in loopsInformationalAcknowledged - 04/22/2025
Minor spelling inconsistencies in error documentationInformationalSolved - 04/22/2025

8. Findings & Tech Details

8.1 Average price desynchronizes after external vault share moves

//

High

Description

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.


Code Location

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);
    }

BVSS
Recommendation

It is recommended to restrict the transfer of vault share tokens to prevent inconsistencies in the avgPrice tracking.

Remediation Comment

SOLVED: The issue was proactively identified at an early stage by the BSX team and successfully addressed in the specified commit id.

Remediation Hash

8.2 Incorrect price tracking after asset consolidation from subaccount deletion

//

High

Description

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.


Code Location

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;
            }
        }
    }

BVSS
Recommendation

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.

Remediation Comment

SOLVED: The BSX team solved the issue in the commit ids 60070e0 and 3e0488b.

Remediation Hash

8.3 Unrestricted acquisition of vault shares by subaccounts during swaps

//

High

Description

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.


Proof of Concept

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:



BVSS
Recommendation

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.

Remediation Comment

SOLVED: The BSX team solved the issue in the commit ids 3e0488b and 89f4d4b.

Remediation Hash

8.4 Account can be self-converted into a subaccount

//

Medium

Description

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.


Code Location

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;

BVSS
Recommendation

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.

Remediation Comment

SOLVED: The BSX team solved the issue in the specified commit id.

Remediation Hash

8.5 Nonces consumed on failed operations prevent retries

//

Medium

Description

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.


Code Location

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
                    );
                }
            }
        }
    }

BVSS
Recommendation

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.

Remediation Comment

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.


8.6 Subaccount deletion can be indefinitely blocked by open positions

//

Medium

Description

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.


Code Location

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);
        }

BVSS
Recommendation

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.

Remediation Comment

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.


8.7 Revoked signers can be reauthorized with old consent signatures

//

Medium

Description

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.


Code Location

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);
    }

BVSS
Recommendation

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.

Remediation Comment

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.


8.8 Unverified vault swap outputs can lead to inconsistent asset accounting

//

Medium

Description

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.


Code Location

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));
        }

BVSS
Recommendation

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.

Remediation Comment

SOLVED: The BSX team solved the issue in the specified commit id.

Remediation Hash

8.9 Unsupported asset pair aborts swap execution

//

Low

Description

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.


Code Location

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);
        }

BVSS
Recommendation

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.

Remediation Comment

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.


8.10 Improper balance management due to missing token authorization checks

//

Low

Description

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.


Code Location

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);
    }

BVSS
Recommendation

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.

Remediation Comment

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.


8.11 Residual token approval could remain after deposits

//

Low

Description

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.


Code Location

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;
    }

BVSS
Recommendation

It is recommended to reset the token approval back to zero immediately after the deposit call so that no leftover allowance remains.

Remediation Comment

SOLVED: The BSX team solved the issue in the specified commit id.

Remediation Hash

8.12 Unbounded supported‑token list loops may exhaust gas

//

Informational

Description

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...
}

BVSS
Recommendation

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.

Remediation Comment

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.


8.13 Suboptimal gas usage due to post-increment in loops

//

Informational

Description

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


BVSS
Recommendation

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.

Remediation Comment

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.


References
contracts/exchange/lib/logic/AccountLogic.sol#L53, L113, L138

8.14 Minor spelling inconsistencies in error documentation

//

Informational

Description

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.

BVSS
Recommendation

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.

Remediation Comment

SOLVED: The BSX team solved the issue in the specified commit id.

Remediation Hash
References
contracts/exchange/lib/Errors.sol#L36, L188

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.

© Halborn 2025. All rights reserved.