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.
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.
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
createDepositcan be front run by a malicious party taking all of their deposit.Vulnerability Detail
In
exchangeRouter, thecreateDepositfunction states the following: @dev Creates a new deposit with the given long token, short token, long token amount, short tokencreateDeposit()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
recordTranferInsimply checks the balance increased in the contracts and credits that to whoever first callscreateDeposit. 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.LoC: https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/bank/StrictBank.sol#L36-L45
Tool used
Manual Review
Recommendation
The
createDepositcode could call the ERC20 transfers within the scope of the function to avoid this ever being a nasty scenario that could take place.