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

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

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

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

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

Reference: OpenZeppelin writing-upgradeable

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

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.

It is recommended to initialize all inherited upgradable contracts.

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

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

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

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.

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

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

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

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

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

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

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

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

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

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

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.

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

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

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

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

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.

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.

ACKNOWLEDGED: The BSX team acknowledged this finding.

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.

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.

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