Zaros Part 1

Summary

The depositMargin() function lacks proper identification of the target account of the deposit, leading to successful re-org attacks which can send the funds accidentally or maliciously to another trading account.

Vulnerability Details

When an account is created through createTradingAccount() it is identified solely by an incrementing id counter called tradingAccountId() .

Reference to code: link

After an account is created the next logical function to call is depositMargin() , which adds collateral so that the account can create market orders. It uses the generated tradingAccountId of the account from above to identify it and deposit the assets to it.

Reference to code: link

Problem is that between the moment they sent the depositMargin() TX and the moment that TX is executed, a block reorg can take place. When it occurs, the account corresponding to that ID may change to another account. For example.

Chain reorgs are very prevalent in L2s, where the contract is planned to be deployed. It is incorrect to assume users will wait until it achieved finality, because there's no warnings to identify this as a threat. Therefore, it remains a very valid concern with reasonable hypotheticals.

Possible flow:

• Alice creates Account 10

• Bob creates Account 11

• Alice send a transaction to deposit 1000 USDz for Account 10

• Block re-org occurs

• Bob account creation TX is moved before Alice’s - now Account 10 belongs to Bob

• Alice TX executes and deposits 1000 USDz in Bobs account instead of her own

Impact

Loss of deposits for honest participants.

Tools Used

Manual Review

Recommended Mitigation

Consider updating the account id to include something that would make it unique and prevent the above from happening - like for example the owner address.

Alternatively update the depositMargin() to additionally take owner parameter and compare that with the actual owner of the account .