Subaccount & Yield - BSX Exchange

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

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.

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

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.

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

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;

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.

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

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.

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

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.

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

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.

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

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.

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

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.

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

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.

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

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.