Contracts Core and BSX Token (2nd round) - BSX Exchange


Prepared by:

Halborn Logo

HALBORN

Last Updated 06/15/2025

Date of Engagement: October 9th, 2024 - October 23rd, 2024

Summary

100% of all REPORTED Findings have been addressed

All findings

11

Critical

0

High

1

Medium

4

Low

3

Informational

3


1. Introduction

BSX engaged Halborn to conduct a security assessment on their smart contracts beginning on October 9th, 2024 and ending on October 23rd, 2024. The security assessment was scoped to the BSX Exchange 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

Halborn was provided two weeks for the engagement and assigned one full-time security engineer to review the security of the smart contract in scope. The engineer is a blockchain and smart contract security expert with advanced penetration testing and smart contract hacking skills, and deep knowledge of multiple blockchain protocols.

The purpose of the assessment is to:

    • Identify potential security issues within the smart contracts.

    • Ensure that smart contract functionality operates as intended.


In summary, Halborn identified some improvements to reduce the likelihood and impact of risks, which were partially addressed by the BSX team. The main identified issues were the following:

    • Guarantee solvency when opening new positions

    • Disable intializers in contracts constructors

    • Initialize inherited contracts

    • Take all inputs into account when signing payloads to guarantee total integrity

    • Keep the storage variable in the same place between two versions of a contract

    • Prevent integer silent signed to unsigned casting

3. Test Approach and Methodology

Halborn performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of this assessment. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of the contracts' solidity code 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 architecture and purpose.

    • Smart contract manual code review and walk-through.

    • Manual assessment of use and safety for the critical Solidity variables and functions in scope to identify any arithmetic-related vulnerability classes.

    • Local testing with custom scripts (Foundry).

    • Fork testing against main networks (Foundry).

    • Static analysis of security for scoped contract, and imported functions

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

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

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

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

5. SCOPE

Files and Repository
(a) Repository: contracts-core
(b) Assessed Commit ID: d932bdf
(c) Items in scope:
  • /1000x/BSX1000x.sol
  • /1000x/interfaces/IBSX1000x.sol
  • /exchange/ClearingService.sol
↓ Expand ↓
Out-of-Scope: Third party dependencies and economic attacks.
Files and Repository
(a) Repository: bsx-token
(c) Items in scope:
  • BsxToken.sol
Out-of-Scope: Third party dependencies and economic attacks.
Out-of-Scope: New features/implementations after the remediation commit IDs.

6. Assessment Summary & Findings Overview

Critical

0

High

1

Medium

4

Low

3

Informational

3

Security analysisRisk levelRemediation Date
BSX1000 solvency not guaranteedHighSolved - 10/27/2024
Missing constructor disabling initializationMediumSolved - 10/27/2024
Missing initialization of inherited upgradable contractsMediumSolved - 10/27/2024
Payloads to sign do not include all input parametersMediumRisk Accepted - 10/27/2024
Removing storage variables in upgradable contractsMediumRisk Accepted - 10/27/2024
Integer unsafe casting issuesLowSolved - 10/27/2024
Order fees are charged twiceLowRisk Accepted - 10/27/2024
Truncated balance not accrued to fund balance after withdrawalLowRisk Accepted - 10/27/2024
ETH deposit does not validate the amount sentInformationalSolved - 10/27/2024
High fees can block the closing position processInformationalAcknowledged - 10/27/2024
Confusing error messagesInformationalSolved - 10/27/2024

7. Findings & Tech Details

7.1 BSX1000 solvency not guaranteed

//

High

Description

The BSX1000x contract facilitates leveraged trading positions, allowing users to open and close positions with profit and loss (PnL) calculations. The gains are taken from the contract's funds, and the losses are added to the funds.


It was identified that the contract lacks a solvency check to ensure that the fund balance is sufficient to cover potential maximum liabilities (PnL) arising from open positions. This introduces a serious financial vulnerability, where the platform could become insolvent, resulting in the inability to meet withdrawal or settlement obligations.


Such a scenario could arise when many positions are opened in a sudden winning move for them, draining the contracts funds until it is not possible to pay off the remaining closing positions.

Proof of Concept

The following unit tests try to open a position even if there are no funds in the contract. The order is created successfully, and a winning PnL triggers an error when trying to close the position because the contract do not hold enough funds to pay off the account:

function test_halborn_forceClosePosition_long_takeProfit() public {
    (address account, uint256 accountKey) = makeAddrAndKey("account");
    (, uint256 signerKey) = makeAddrAndKey("signer");
    _authorizeSigner(accountKey, signerKey);

    uint256 accountBalance = 100 * 1e18;
    _deposit(account, accountBalance);

    // open position
    BSX1000x.Order memory order;
    order.productId = 1;
    order.account = account;
    order.nonce = 5;
    order.margin = 10 * 1e18;
    order.leverage = 1000 * 1e18;
    order.size = 1 * 1e18;
    order.price = 10_000 * 1e18;
    order.takeProfitPrice = 10_020 * 1e18;
    order.liquidationPrice = 9990 * 1e18;
    order.fee = 2e18;
    bytes memory openSignature = _signOpenOrder(signerKey, order);
    vm.expectEmit();
    emit IBSX1000x.OpenPosition(order.productId, order.account, order.nonce, order.fee);
    bsx1000x.openPosition(order, openSignature);

    // force close position
    int256 pnl = 20 * 1e18;
    int256 closePositionFee = 1e18;

    IBSX1000x.Balance memory accountBalanceBefore = bsx1000x.getBalance(account);
    uint256 fundBalanceBefore = bsx1000x.fundBalance();

    bsx1000x.forceClosePosition(
        order.productId, order.account, order.nonce, pnl, closePositionFee, IBSX1000x.ClosePositionReason.TakeProfit
    );

    IBSX1000x.Position memory position = bsx1000x.getPosition(account, order.nonce);
    IBSX1000x.Balance memory accountBalanceAfter = bsx1000x.getBalance(account);
    uint256 fundBalanceAfter = bsx1000x.fundBalance();

    assertEq(fundBalanceAfter, fundBalanceBefore - uint256(pnl) + uint256(closePositionFee));
    assertEq(
        accountBalanceAfter.available,
        accountBalanceBefore.available + uint256(pnl) - uint256(closePositionFee) + order.margin
    );
    assertEq(accountBalanceAfter.locked, 0);
    assertEq(
        fundBalanceBefore + accountBalanceBefore.available + accountBalanceBefore.locked,
        fundBalanceAfter + accountBalanceAfter.available
    );

    // check position state
    assertEq(position.productId, order.productId);
    assertEq(position.margin, order.margin);
    assertEq(position.leverage, order.leverage);
    assertEq(position.size, order.size);
    assertEq(position.openPrice, order.price);
    assertEq(position.closePrice, order.takeProfitPrice);
    assertEq(position.takeProfitPrice, order.takeProfitPrice);
    assertEq(position.liquidationPrice, order.liquidationPrice);
    assertEq(uint8(position.status), uint8(IBSX1000x.PositionStatus.TakeProfit));

    // Log fund balance before and after
    console.log("Fund balance before: %d", fundBalanceBefore);
    console.log("Fund balance after: %d", fundBalanceAfter);
}

Running that test shows the following error:

Failing tests:
Encountered 1 failing test in test/unit/BSX1000x.t.sol:BSX1000xTest
[FAIL: InsufficientFundBalance()] test_halborn_forceClosePosition_long_takeProfit() (gas: 347219)
BVSS
Recommendation

It is recommended to only allow positions if there is enough balance to pay off the user.

Remediation Comment

SOLVED: The BSX team fixed this issue by adding a dedicated variable tracking the balance potentially owed to the future payoffs.

7.2 Missing constructor disabling initialization

//

Medium

Description

The BSX1000x contract follows the OpenZeppelin upgradeable contract pattern but does not call _disableInitializers(). This introduces a re-initialization vulnerability, if the initialize function is not called during the initial deployment or is re-exploited through an upgrade, any actor can reinitialize the contract, allowing them to rewrite sensitive state variables such as the Access contract address, or the collateral token address.


An attacker could exploit this vulnerability to replace the contracts by malicious ones, potentially enabling the draining of the BSX1000x contract.


This vulnerability applies to the following contracts:

  • BSX1000x

  • Access

  • ClearingService

  • OrderBook

  • Exchange

  • Perp

  • Spot

Proof of Concept

The following test proves that if the contract is not initialized during the deployment, anyone could call the initialize function:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.23;

import {Test, console} from "forge-std/Test.sol";
import {MathLite} from "./libraries/MathLite.sol";
import { InitializableCounter } from "../src/InitializableCounter.sol";
import "lib/openzeppelin-foundry-upgrades/src/Upgrades.sol";

contract InitializableCounter is Initializable {
    uint256 public value;

    function initialize(uint256 _setValue) public initializer {
        value = _setValue;
    }
}

contract Upgrading is Test {
    InitializableCounter public counter;
    address public implementation;

    function setUp() public {
        Options memory deploy;
        deploy.constructorData = "";

        address proxy = Upgrades.deployUUPSProxy(
            "InitializableCounter.sol:InitializableCounter", 
             "" // Initialization not called during deployment
        );
        address implementation = Upgrades.getImplementationAddress(proxy);

        counter = InitializableCounter(proxy);
    }

    function test_proper_intialization() public {
        console.log("Counter value", counter.value());
    }

    function test_attacker_initialize() public {
        address attacker = vm.addr(0x01010101);
        vm.prank(attacker);
        counter.initialize(1337);

        console.log("Counter value", counter.value());
    }
}

The test results show that the attacker successfully modified the contract state by calling the intialize function on the implementation.

[PASS] test_attacker_initialize() (gas: 63881)
Logs:
  Counter value 1337

[PASS] test_proper_intialization() (gas: 15590)
Logs:
  Counter value 0
BVSS
Recommendation

It is recommended to add the following call to the BSX1000 contract:

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
    _disableInitializers();
}

Reference: OpenZeppelin writing-upgradeable

Remediation Comment

SOLVED: The BSX team fixed this issue by removing the intialize functions, as the contracts were already deployed and initialized.

7.3 Missing initialization of inherited upgradable contracts

//

Medium

Description

The Exchange contract inherits from OwnableUpgradeable and ReentrancyGuardUpgradeable, but it does not call their initializers during the initialize() process. As a result, the contract lacks ownership control and reentrancy protection, leaving it vulnerable to unauthorized function execution and reentrancy attacks.


Since the contract follows the OpenZeppelin upgradeable pattern, it must explicitly call the __Ownable_init() and __ReentrancyGuard_init() functions in the initialize() function to properly initialize those modules. Without this initialization, critical security measures are bypassed, compromising the contract’s security and access control.


This unsafe pattern is repeated in the following contracts:

  • Access: AccessControlUpgradeable

  • ClearingService: OwnableUpgradeable

  • Exchange: EIP712Upgradeable, OwnableUpgradeable, ReentrancyGuardUpgradeable

  • OrderBook: OwnableUpgradeable

  • Perp: OwnableUpgradeable

  • Spot: OwnableUpgradeable


Note that the inherited OwnableUpgrade contracts are not used, as it was found no instance of onlyOwner in the codebase.

BVSS
Recommendation

It is recommended to initialize all inherited upgradable contracts.

Remediation Comment

SOLVED: The BSX team fixed this issue by removing the intialize functions, as the contracts were already deployed and initialized.

7.4 Payloads to sign do not include all input parameters

//

Medium

Description

Cryptographic signatures

Cryptographic signatures ensure that a message has not been tampered with and comes from a known sender. The signature creation and verification process is as follows:

  1. Hash Creation:
    The sender takes all important parameters (like the public key of the sender and the amount to spend) and creates a hash of this data:
    h = hash(sender_public_key || amount_to_spend)
    This hash acts as a summary of the important information for verification.

  2. Signing the Hash:
    The sender uses their private key to create a cryptographic signature of the hash:
    s = sign(private_key, h)
    This signature ensures the integrity and authenticity of the data.

  3. Verification by the Recipient:
    When the recipient receives the signature s along with the original parameters (sender_public_key and amount_to_spend), they can perform the following steps:

    • Recreate the original hash using the parameters:
      h = hash(sender_public_key || amount_to_spend)

    • Verify the signature s using the sender’s public key and the hash:
      verified_public_key = verify(h, s)

  4. Validation:
    If the extracted public key from the signature matches the claimed sender_public_key, it confirms that the message came from the legitimate sender and was not altered:
    assert(sender_public_key == verified_public_key)

This process guarantees that the sender is who they claim to be, and that the content was not modified in transit.

BSX1000 withdraw

The withdraw function of the BSX1000 contract is defined as:

function withdraw(address account, uint256 amount, uint256 fee, uint256 nonce, bytes memory signature)

It would be expected to have all (account, amount, fee, nonce) included in the hash, however it was found that the fee parameter was excluded from the reconstruction:

bytes32 withdrawHash = _hashTypedDataV4(keccak256(abi.encode(WITHDRAW_TYPEHASH, account, amount, nonce)));
if (!SignatureChecker.isValidSignatureNow(account, withdrawHash, signature)) {
    revert InvalidSignature(account);
}

Therefore, the fee parameter could be accepted for any value, without the expectation of the signer. A mitigation is already in place, preventing the fee to be greater than MAX_WITHDRAWAL_FEE.


BSX1000 open_position

The openPosition function in the BSX1000 contract allows users to open trading positions. It is defined as:

function openPosition(Order calldata order, uint256 credit, bytes memory signature)
    public
    onlyRole(access.BSX1000_OPERATOR_ROLE())
{
    bytes32 orderHash = _hashTypedDataV4(
        keccak256(
            abi.encode(
                OPEN_POSITION_TYPEHASH, order.productId, order.account, order.nonce, order.margin, order.leverage
            )
        )
    );
    _validateAuthorization(order.account, orderHash, signature); // ensures account approved sender as signer
    // ...
}

The order hash used for signature verification is missing parameters:

  • size,

  • price,

  • takeProfitPrice,

  • liquidationPrice, and

  • fee


The contract fails to guarantee that the parameters signed off-chain by the user are the same as those used in the actual transaction.


As a reference, the Order structure:

struct Order {
    uint32 productId;
    address account;
    uint256 nonce;
    uint128 leverage;
    uint128 margin;
    int128 size; // positive for long, negative for short
    uint128 price;
    uint128 takeProfitPrice;
    uint128 liquidationPrice;
    int256 fee;
}

BSX1000 close_position

Likewise, the close_position function does not factor all arguments of the function in the hashed payload:

function closePosition(
    uint32 productId,
    address account,
    uint256 nonce,
    uint128 closePrice,
    int256 pnl,
    int256 fee,
    bytes memory signature
) external onlyRole(access.BSX1000_OPERATOR_ROLE()) {
    bytes32 closeOrderHash =
        _hashTypedDataV4(keccak256(abi.encode(CLOSE_POSITION_TYPEHASH, productId, account, nonce)));
    _validateAuthorization(account, closeOrderHash, signature);
    // ...
}

Exchange withdraw

In the exchange contract, the withdraw operation in the _handleOperation function does not include the fee parameter:

bytes32 digest =
    _hashTypedDataV4(keccak256(abi.encode(WITHDRAW_TYPEHASH, txs.sender, txs.token, txs.amount, txs.nonce)));
if (!UNIVERSAL_SIG_VALIDATOR.isValidSig(txs.sender, digest, txs.signature)) {
    emit WithdrawFailed(txs.sender, txs.nonce, 0, 0);
    return;
}
BVSS
Recommendation

It is recommended to include all input parameters in the payload to sign, in order to guarantee integrity of the parameters.

Remediation Comment

RISK ACCEPTED: The BSX team accepted the risk, mentioning that We want to clarify that the dynamic fee structure is intentional by design. However, we have implemented smart contract checks to ensure that submitted fees cannot exceed the maximum allowable threshold.

7.5 Removing storage variables in upgradable contracts

//

Medium

Description

Exchange

The Exchange contract is upgradeable, which means it relies on the memory layout of storage variables across contract versions. From the provided comparison between two versions of the contract, storage variables have been removed or replaced. In Solidity upgradeable contracts, changing the storage layout improperly can lead to serious vulnerabilities, and violating any of the storage layout restrictions will cause the upgraded version of the contract to have its storage values mixed up, and can lead to critical errors in the protocol.


The current layout is as follows:

IClearingService public clearingService;
ISpot public spotEngine;
IPerp public perpEngine;
IOrderBook public book;
Access public access;

EnumerableSet.AddressSet private supportedTokens;
mapping(address account => mapping(address signer => bool isAuthorized)) private _signingWallets;
mapping(address token => uint256 amount) private _collectedFee;
mapping(address account => mapping(uint64 registerSignerNonce => bool used)) public isRegisterSignerNonceUsed;
mapping(address account => mapping(uint256 liquidationNonce => bool liquidated)) public isLiquidationNonceUsed;

address public universalRouter;
uint256 private _lastResetBlockNumber; // deprecated
int256 private _sequencerFee;
EnumerableSet.AddressSet private _userWallets; // deprecated
uint256 public lastFundingRateUpdate;
uint32 public executedTransactionCounter;
address public feeRecipientAddress;
bool private _isTwoPhaseWithdrawEnabled; // deprecated
bool public canDeposit;
bool public canWithdraw;
bool public pauseBatchProcess;
mapping(address account => mapping(uint64 withdrawNonce => bool used)) public isWithdrawNonceUsed;
mapping(address account => bool isRequesting) private _isRequestingTwoPhaseWithdraw; // deprecated

While the precedent layout (see commit 8690cdc) was:

IClearingService public clearingService;
ISpot public spotEngine;
IPerp public perpEngine;
IOrderBook public book;
IFee public fee;
Access accessContract;

EnumerableSetUpgradeable.AddressSet supportedTokens;
mapping(address => mapping(address => bool)) public signingWallets;
mapping(uint256 => WithdrawalInfo) public withdrawalInfo;
mapping(address => mapping(uint64 => bool)) public usedNonces;
uint256 public withdrawalRequestIDCounter;
uint256 public forceWithdrawalGracePeriodSecond;
uint256 public lastResetBlockNumber;
int256 sequencerFee;
EnumerableSetUpgradeable.AddressSet userWallets;
uint256 public lastFundingRateUpdate;
uint32 public executedTransactionCounter;
address public feeRecipientAddress;
bool public isTwoPhaseWithdrawEnabled;
bool public canDeposit;
bool public canWithdraw;

Therefore, many slots of the layout have changed during the upgrade, which would cause serious problems when upgrading the contract.


OrderBook

The OrderBook contract also removed one slot from the previous version, IFee public fee (see commit 8690cdcc).

BVSS
Recommendation

It is recommended to keep the same layout as the first version and only extends by adding variables after the last declared variable.

Remediation Comment

RISK ACCEPTED: The BSX team accepted the risk, mentioning that it aligns with our design requirements and risk assessment.

7.6 Integer unsafe casting issues

//

Low

Description

In solidity, casting a signed integer to an unsigned integer will not trigger any error even if the signed integer was negative, causing the unsigned representation to result in a very high number. This is an effect of the most significant byte set to one to represent a negative number.


This potential issue was located in various places in the codebase, enumerated below:

ClearingService

The deposit and the withdraw functions are affected:

function deposit(address account, uint256 amount, address token, ISpot spotEngine) external onlySequencer { // @audit any spot engine here
    ISpot.AccountDelta[] memory productDelta = new ISpot.AccountDelta[](1);
    productDelta[0] = ISpot.AccountDelta(token, account, int256(amount)); 
    spotEngine.modifyAccount(productDelta);
    spotEngine.setTotalBalance(token, amount, true);
}

function withdraw(address account, uint256 amount, address token, ISpot spotEngine) external onlySequencer {
    ISpot.AccountDelta[] memory productDelta = new ISpot.AccountDelta[](1);
    productDelta[0] = ISpot.AccountDelta(token, account, -int256(amount));    spotEngine.modifyAccount(productDelta);
    spotEngine.setTotalBalance(token, amount, false);
}

If the amount most significant byte is 1, the integer casting would result in a negative number without reverting. In this case, it is unlikely to happen because the deposit would be huge (at least 2**255 tokens) to trigger the casting underflow.


Exchange

In the withdraw branch of the _handleOperation function, an unsafe integer casting from unsigned to signed was identified, potentially allowing to bypass a balance check verifying that the user has enough balance to withdraw the desired amount. For example, the result of the unsafe casting could be negative, therefore always being inferior to the current balance, and passing the check. However, the rest of the function includes an additional check that prevents any undesirable logic to happen.


int256 currentBalance = balanceOf(txs.sender, txs.token);
if (currentBalance < int256(int128(txs.amount))) {
    emit WithdrawFailed(txs.sender, txs.nonce, txs.amount, currentBalance);
    return;
} else {
    clearingService.withdraw(txs.sender, txs.amount, txs.token, spotEngine);
    IERC20Extend product = IERC20Extend(txs.token);
    uint256 netAmount = txs.amount - txs.withdrawalSequencerFee;
    uint256 amountToTransfer = netAmount.convertFromScale(txs.token);
    _collectedFee[txs.token] += txs.withdrawalSequencerFee;
    product.safeTransfer(txs.sender, amountToTransfer);
    int256 afterBalance = balanceOf(txs.sender, txs.token);
    emit WithdrawSucceeded(
        txs.token, txs.sender, txs.nonce, txs.amount, uint256(afterBalance), txs.withdrawalSequencerFee
    );
}
BVSS
Recommendation

It is recommended to verify ahead of the casting and catch underflow or overflow errors

Remediation Comment

SOLVED: The BSX team fixed this issue by using the MathHelper library, throwing an error when an overflow occurs during integer casting.

7.7 Order fees are charged twice

//

Low

Description

The BSX1000x contract applies fees both during the opening and closing of positions, which can be considered an unintended or excessive charge. In leveraged trading systems, it is typically expected to either:


  1. Apply the fee only during the opening of a position, or

  2. Apply it only at the time of closing, reflecting the trading outcome (PnL) and the overall transaction.


Charging fees at both stages (opening and closing) could be an overlook on the intended design, resulting in users being charged more than expected, discouraging usage and reducing the attractiveness of the platform.


Fee during position opening:

// Inside openPosition()
int256 newBalance = (accountBalance.available.safeInt256() - order.fee);
if (newBalance < 0) {
    revert InsufficientAccountBalance();
}
accountBalance.available = newBalance.safeUInt256();

fundBalance = (fundBalance.safeInt256() + order.fee).safeUInt256();

Fee during position closing:

// Inside closePosition() and forceClosePosition()
if (fee > position.margin.safeInt128() || fee > position.margin.safeInt128() + pnl) {
    revert InvalidOrderFee();
}
_updateBalanceAfterClosingPosition(account, pnl, fee);

// _updateBalanceAfterClosingPosition()
int256 updatedAccountBalance = accountBalance.available.safeInt256() + pnl - fee;
int256 newFundBalance = fundBalance.safeInt256() - pnl + fee;
fundBalance = newFundBalance.safeUInt256();
BVSS
Recommendation

It is recommended to review the intended design and remove the double fee if needed.

Remediation Comment

RISK ACCEPTED: The BSX team accepted the risk, mentioning that it aligns with our design requirements and risk assessment.

7.8 Truncated balance not accrued to fund balance after withdrawal

//

Low

Description

BSX1000x

The withdraw function incorrectly handles fee accrual due to the loss of precision when scaling token amounts. Specifically, the function calculates netAmount (in base 18 precision) and then scales it down to amountToTransfer (in base 6 precision). However, any truncated portion of the original amount (the difference between the base 18 and base 6 values) is not added to the fund balance, causing a loss in expected accrual.

function withdraw(address account, uint256 amount, uint256 fee, uint256 nonce, bytes memory signature)
    public
    onlyRole(access.GENERAL_ROLE())
{
    uint256 netAmount = amount - fee;
    uint256 amountToTransfer = netAmount.convertFromScale(address(collateralToken));
    if (amount == 0 || amountToTransfer == 0) revert ZeroAmount();

    // ...

    uint256 newBalance = accountBalance - amount;
    _balance[account].available = newBalance;

    // ....

    fundBalance = fundBalance + fee; 

    collateralToken.safeTransfer(account, amountToTransfer);
}

Any leftover decimals from the scaling operation (e.g., 12.345678901234567890 - 12.345678) are ignored and not recorded to the fund balance, suffering from a minor loss in fee accrual: The fund balance will be slightly lower than expected, and gradually more overtime.


For reference, the _convertFromScale function:

function _convertFromScale(uint256 scaledAmount, uint8 decimals) internal pure returns (uint256) {
    return (scaledAmount * 10 ** decimals) / FACTOR_SCALE;
}

Exchange

The claimSequencerFees function possibly truncates the collected fees and sends that amount without keeping information on the remained amount, that will be stuck in the contract.

function claimSequencerFees() external onlyRole(access.GENERAL_ROLE()) {
    address underlyingAsset = book.getCollateralToken();

    for (uint256 i = 0; i < supportedTokens.length(); ++i) {
        address token = supportedTokens.at(i);
        uint256 totalFee = _collectedFee[token];
        if (token == underlyingAsset) {
            totalFee += book.claimSequencerFees().safeUInt256();
        }
        _collectedFee[token] = 0;

        uint256 amountToTransfer = totalFee.convertFromScale(token);
        IERC20Extend(token).safeTransfer(feeRecipientAddress, amountToTransfer);
        emit ClaimSequencerFees(msg.sender, token, totalFee);
    }
}

The truncation also happens in claimTradingFees:

function claimTradingFees() external onlyRole(access.GENERAL_ROLE()) {
    address token = book.getCollateralToken();
    uint256 totalFee = uint256(book.claimTradingFees());
    uint256 amountToTransfer = totalFee.convertFromScale(token);
    IERC20Extend(token).safeTransfer(feeRecipientAddress, amountToTransfer);
    emit ClaimTradingFees(msg.sender, totalFee);
}
BVSS
Recommendation

For the BSX1000x contract, it is recommended to adjust the fee accrual logic to capture the truncated portion and add it to the fund balance. For example:

uint256 truncatedAmount = netAmount - amountToTransfer.convertToScale(address(collateralToken)); 
fundBalance = fundBalance + fee + truncatedAmount;

For the Exchange contract, it is recommended to keep the remainder value in storage to attempt to send it in the next collect round.

Remediation Comment

RISK ACCEPTED: The BSX team accepted the risk, mentioning that it aligns with our design requirements and risk assessment.

7.9 ETH deposit does not validate the amount sent

//

Informational

Description

The deposit() function in the Exchange contract allows users to deposit ERC20 tokens and ETH. However, when ETH is deposited, the function does not validate the msg.value against the specified amount parameter. If the caller sends more ETH than intended, the excess ETH will be irrecoverable unless refunded manually, leading to unexpected loss of funds.


The deposit function:

function deposit(address recipient, address token, uint128 amount) public payable supportedToken(token) {
    if (!canDeposit) revert Errors.Exchange_DisabledDeposit();
    if (amount == 0) revert Errors.Exchange_ZeroAmount();

    if (token == NATIVE_ETH) { 
        token = WETH9;
        IWETH9(token).deposit{value: amount}();
    } else {
        (uint256 roundDownAmount, uint256 amountToTransfer) = 
            uint256(amount).roundDownAndConvertFromScale(token);

        if (roundDownAmount == 0 || amountToTransfer == 0) revert Errors.Exchange_ZeroAmount();
        amount = uint128(roundDownAmount);
        IERC20Extend(token).safeTransferFrom(msg.sender, address(this), amountToTransfer);
    }

    clearingService.deposit(recipient, amount, token, spotEngine);
    int256 currentBalance = balanceOf(recipient, token);
    emit Deposit(token, recipient, amount, uint256(currentBalance));
}
BVSS
Recommendation

It is recommended to include a check between the msg.value and the amount argument.

Remediation Comment

SOLVED: The BSX team fixed this finding, by refusing transactions that do not carry the exact amount.

7.10 High fees can block the closing position process

//

Informational

Description

In the closePosition function, there is a restriction that the fee cannot exceed the margin or the margin plus profit-and-loss (PnL). This constraint can block position closures if the fee becomes higher than the available margin.

if (fee > position.margin.safeInt128() || fee > position.margin.safeInt128() + pnl) {
    revert InvalidOrderFee();
}

This scenario is unlikely since there are mechanisms to limit the upper bound of fees.

BVSS
Recommendation

Implement a fallback mechanism that adjusts the fee to the maximum available margin, allowing the position to close even if the fee exceeds the margin or PnL, preventing transaction blockage.

Remediation Comment

ACKNOWLEDGED: The BSX team acknowledged this finding.

7.11 Confusing error messages

//

Informational

Description

The current implementation of the BSX1000 contract uses conversions between int256 and uint256 for handling balances and fees. Specifically, the safeUInt256() function can revert with a generic error message (e.g., InvalidUint256), which provides little insight into the actual issue. In cases like insufficient balances, a more specific and informative error message (e.g., InsufficientFundBalance) is needed to clearly communicate the problem.


The affected line of code is:

fundBalance = (fundBalance.safeInt256() + order.fee).safeUInt256();

The issue could arise when all of the fund balance is already utilized for margins and a negative fee is given to the user opening the position.

BVSS
Recommendation

It is recommended to prevent safeUInt256() from reverting by checking ahead and reverting with more descriptive error messages like InsufficientFundBalance to make it clear what the issue is.

Remediation Comment

SOLVED: The BSX team fixed this issue by including an early check, throwing a more descriptive error.

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.