SEV 10: Multiple branches have initializers that use same storage slot to determine initialization state of the contract

Severity: Low

Summary

The protocol has multiple branches with initialization functions, all using a single initialized variable. As a result, all branches must be initialized in the constructor otherwise the initialization fails. and the future branches should not use initializer modifier of Initializable library.

Vulnerability Details

The GlobalConfigurationBranch, SettlementBranch, and UpgradeBranch branches have initialize functions marked with initializer modifier of OZ Initializable library.

Because Initializable uses a static storage slot for initialized flag and all the branches refer the same storage, the first initialization function will mark the contract as initialiazed. It should not be possible to initialize rest of the branches.

However, the OZ Initializable contract allows execution of multiple initialization functions in the constructor for testing purposes.

Definition of initializer modifier: openzeppelin-contracts/contracts/proxy/utils/Initializable.sol#L98-L120

This allows initializing all the branches if all the branches are added in the constructor of RootProxy. If any of the branch is not added in the constructor then the contract needs to be redeployed.

Impact

The branches cannot be initialized if are not initialized in the constructor. Future branches cannot use initializer modifier.

Tools Used

Manual Review

Recommendations

Store initialized flag per branch. Otherwise, consider using reinitializer with incremental values for initializable functions.

SEV 11: Precision loss in the conversion of token amount into its decimals

Severity: Low

Summary

All computations are performed with 18 decimals in the system. The token amounts are converted to token decimals before calling the token transfer function. This conversion results in precision loss resulting transfer of less value.

The conversion is performed while deducting margin to pay for fees, and to cover for the debt when an account is liquidated. The receiver is always the protocol: protocol controlled keepers receive order, settlement and liquidation fees, and protocol when account is liquidated.

Considering the system supports WBTC token which has 8 decimals and is of significant value, the losses add up from multiple interactions and become non-negligible.

Vulnerability Details

The TradingAccount::withdrawMarginUsd function is used to deduct from trader's collateral

Snippet of withdrawMarginUsd performing the conversions: TradingAccount.sol#L451-L467

The withdrawMarginUsd function calculates the amount to be deducted in 18 decimals. Because the token might have different decimals the amount is converted into the token amount using the MarginCollateralConfiguration::convertUd60x18ToTokenAmount function.

Definition of the MarginCollateralConfiguration::convertUd60x18ToTokenAmount function:
MarginCollateralConfiguration.sol#L57-L63

The conversion truncates the lower SYSTEM_DECIMALS - TOKEN_DECIMALS leading to precision loss. The withdrawMarginUsdfunction still considers before the conversion as the value transfer.

POC:

Impact

Keepers receive lesser fee than the actual because of precision loss. The difference might become significant in the long run based on the amount of trades executed.

Tools Used

Manual Review

Recommendations

Updated the if condition in TradingAccount::withdrawMarginUsd with the following:

SEV 12: TradingAccountBranch::createTradingAccountAndMulticall is payable and uses delegateCall in a loop

Severity: Low

Summary

The TradingAccountBranch::createTradingAccountAndMulticall function is to allow traders perform multiple operations after creating a trading account in the same transaction.

The createTradingAccountAndMulticall is a payable function and calls self using delegate calls in a loop. As a result, the msg.value will be same for all calls.

• The value transfer happens only once and the contract would consider msg.value as separate transfer for each individual delegate call. Presence of a function that uses msg.value could lead to loss in funds.

• The function cannot be used to call a payable function with non-zero value and the non-payable functions. The depositMargin is the main function that is intended to be called in the function and is non-payable

Definition of the createTradingAccountAndMulticall function: TradingAccountBranch.sol#L285-L311

Vulnerability Details

see summary

Impact

• Addition of a payable function that uses msg.value could lead to funds loss.

• Traders cannot perform a payable operation with non-zero value and non-payable operations.

Tools Used

Manual Review

Recommendations

Remove the payable modifier. Do not use delegateCall in a loop in payable functions.

SEV 13: Incorrect implementation of slippage protection for offline orders

Severity: Low

Summary

The condition used for slippage protection is in reverse:

Vulnerability Details

The offline orders have a targetPrice parameter which is used for slippage protection. It is considered as maximum fill price for buy orders and minimum fill price for sell orders.

The SettlementBranch::fillOffchainOrders validates the order's fill price with the targetPrice.

Slippage protection condition in thefillOffchainOrders function: SettlementBranch.sol#L283-L292

The implementation incorrectly uses targetPrice as minimum fill price for buy orders and as maximum fill price for sell orders.

As a result, slippage protection will prevent orders from filling at "good price" and only allows them to be filled at "bad price". The orders are forced to be filled at bad prices.

Exploit Scenario:

• Trader creates a order to open a Long position on BTC market

• Current fill price = 68018

• Trader sets the targetPrice = 68030

Attempts to fill the trader's order will fail until fill price becomes >= 68030 and the trader buys at a bad price of 68030

Impact

Slippage protection works against the trader. Filling offline orders will fail for orders with reasonable targetPrice. In case the orders succeeds then the order will be filled at a bad price.

Tools Used

Manual Review

Recommendations

Change the condition used for computing isFillPriceValid

SEV 14: OrderBranch::simulateTrade function uses after-trade account profit to compute before-the-trade margin balance

Severity: Low

Summary

The OrderBranch::simulateTrade function ensures the liquidatable accounts cannot create market orders. While computing the account's margin balance, the function uses the estimated pnl after the requested order is processed. However, the function should have used the pnl computed before the trade.

Vulnerability Details

simulateTrade function using the after-the-trade pnl for computing before-the-trade margin balance:

Incorrect snippet of the simulateTrade function: OrderBranch.sol#L121-L137

Every trade influences the markPrice. As a result, the pnl computed after the trade will be different from the before-the-trade. As a result, the OrderBranch::simulateTrade might consider account which is not liquidatable as liquidatable and might consider liquidatable as non-liquidatable.

Impact

Accounts which are not liquidatable might be considered as liquidatable and protocol might reject creation of valid market order.

Tools Used

Manual Review

Recommendations

Store the pnl returned by tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO); on L131 for before the trade pnl. Use this value in the computation of trader's margin balance for performing the account liquidatable check.

SEV 15: The GlobalConfiguration::configureCollateralLiquidationPriority always adds new tokens at the end

Severity: Low

Summary

The collateralLiquidationPriority list is used as the collateral priority queue for liquidation. The list is intended to have volatile and more riskier tokens before stable tokens. Because the configureCollateralLiquidationPriority function always adds the newly supported tokens at the end, the queue might result in liquidating stable tokens first if the new token is volatile.

Definition of the configureCollateralLiquidationPriority function: GlobalConfiguration.sol#L101-L112

Exploit Scenario:

The Zaros developers add support for cbEth token. The configureCollateralLiquidationPriority adds the token at the end after stable tokens such as USDC, USDT. The stable tokens are liquidated first and liquidation might leave volatile-riskier cbEth token in trader's balance.

Vulnerability Details

see summary.

Impact

Liquidation might leave more-riskier, less-stable tokens in trader's collateral if the newly added token is less-stable token.

Tools Used

Manual Review

Recommendations

Update the configureCollateralLiquidationPriority function to have an additional indexes parameter and to insert the new tokens at the specified indexes.