float-audits - User deposits can be lost if deposits are not crafted carefully
Closed this issue · 0 comments
float-audits
high
User deposits can be lost if deposits are not crafted carefully
Summary
A user depositing ERC20 tokens into strict bank, then subsequently calling createDeposit can be front run by a malicious party taking all of their deposit.
Vulnerability Detail
In exchangeRouter, the createDeposit function states the following:
@dev Creates a new deposit with the given long token, short token, long token amount, short token
* amount, and deposit parameters. The deposit is created by transferring the specified amounts of
* long and short tokens from the caller's account to the deposit store, and then calling the
* createDeposit() function on the deposit handler contract.
*
If this process is not carried out atomically through a multicall or some carefully structured contract, once the user sends the specified long or short tokens, another user is able to call createDeposit() before the original user, claiming all of their collateral.
Impact
A user who tried to transfer ERC20 tokens to the deposit store then subsequently call create deposit without carefully crafting a multicall will have their entire deposit lost by a front runner who claims the deposit on their behalf.
Code Snippet
One can see that recordTranferIn simply checks the balance increased in the contracts and credits that to whoever first calls createDeposit. If sending of the ERC20 tokens to this deposit store ever happens non-atomically with the createDeposit call, the user stands to lose all their deposit. This is not stated clearly anywhere and its a conceivable scenario for a user to first transfer their ERC20 tokens before calling create deposit.
Tool used
Manual Review
Recommendation
The createDeposit code could call the ERC20 transfers within the scope of the function to avoid this ever being a nasty scenario that could take place.