[L-01] Missing a dedicated function to deposit Ether

The fee to pay for verifying chainlink data stream reports are calculated and paid in native token.

SettlementConfig.verifyDataStreamReport gets the fee denominated in Ether then calls verifyReport. Next the fee amount is sent to chainlinkVerifier.

The problem is that the only payable function is TradingAccountBranch.createTradingAccountAndMulticall. Each time when admin wants to sent Ether to contract for reports verification, a new tradingAccountId is created.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement an empty receive function

[L-02] If a tradingAccountId is transferred back and forth old pending orders that should be canceled are active again

When a tradingAccountId NFT is transferred old pending orders can not be executed anymore because the new accountId owner is not the signed. FillOffchainOrder is reverted.

The tradingAccount owner is updated in notifyAccounTransfer when the nftId is transferred

While this check makes old pending transactions non executable, if the same tradingAccountId is transferred back to first owner, the old pending orders are active again and can be executed.
Imagine a scenario where an user signs offchian an order.

• Order does not get executed (eg. due to price not matching), he transfer tradingAccountId to another address he controls. User thinks the order is canceled.

• After some time, while no new orders are created (thus nonce doesn't get increased) he transfer same tradingAccountId to first address. => the old unfilled order is active again. Order can be filled when price conditions are meet.

Impact

While traders think old pending positions are canceled, new positions can be created at unfavorable prices.

Recommended Mitigation Steps

Increase the trading account's nonce when the account is transferred.

[L-03] Liquidation can take up many blocks, increasing the chance that protocol will accumulate bad debt

In the liquidation process, checkUpkeep is responsible for identifying liquidatable accountIDs within a specified range (checkLowerBound, checkUpperBound) and passing a subset of these to performUpkeep for execution.

checkUpkeep operates on two distinct account ID intervals:

1. A checking interval (checkLowerBound, checkUpperBound) for liquidation condition evaluation

2. An execution interval (performLowerBound, performUpperBound) for actual liquidation processing

Multiple upkeeps can be registered to process many accounts per block.

The workflow is as follows:

1. checkUpkeep examines an interval of accountIds to identify those meeting the liquidation condition.

2. A subset of liquidatable accounts, constrained by the execution interval, is passed to performUpkeep

3. Each upkeep processes its allocated slice of liquidatable accounts in every block

A potential bottleneck may arise when there are significantly more accounts to be liquidated than the number of actual liquidations defined by the execution interval (performLowerBound, performUpperBound) within a single block.
Let's take the following example:

checkLowerBound = 100

checkUpperBound = 200

performLowerBound = 0

performUpperBound = 10

Checking interval length = 200 - 100 = 100

Execution interval length = 10 - 0 = 10.

If all accounts from checking interval are liquidatable, it needs 10 blocks to liquidate all of them. In this time market conditions can get worse, making user's margin to not cover for PnL and fees.Because worsening market conditions new users became liquidatable adding more pressure (time needed to liquidate all positions) .

Moreover if performLowerBound is different than 0, some accounts will not get liquidated because of the following check:

Recommended Mitigation Steps

1. Renounce to execution interval feature and liquidate all liquidatable accounts found in check interval.
   This means that a smaller check interval need to be configured and more upkeeps to be registred.

2. Renounce to performLowerBound check to ensure no liquidatable accounts are skipped.

[L-04] Unused data is encoded in checkUpkeep

In checkUpkeep the address(this) is encoded in extraData and passed to performUpkeep but never decoded.
There's no issue with this because accountsToBeLiquidated is of type uint128. Instead if ids would have been of type uint256, address(this) would have been interpreted as a valid Id.

In liquidateAccounts() function TradingAccount.loadExisting would reverted because the Liquidation Keeper address (address(this) encoded) would not have an owner.

Recommended Mitigation Steps

Remove the address(this)