Deposits would revert #626
Labels
2 (Med Risk)
Assets not at direct risk, but function/availability of the protocol could be impacted or leak value
bug
Something isn't working
duplicate-104
satisfactory
satisfies C4 submission criteria; eligible for awards
Lines of code
https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Trading.sol#L643-L659
Vulnerability details
Impact
When the margin asset is USDT, after the first deposit all following ones would revert allowing no more trades.
Proof of Concept
The
_handleDeposit()
function in Trading.sol's Trading contract is calling approve() inconditionally at every deposit.The USDT Tethered stablecoin uses a mitigation to avoid approve race conditions which reverts if the allowance is non-zero, as showed in the code snippet below taken from USDT contract on Etherscan:
Tools Used
Manual review
Recommended Mitigation Steps
Either approve the exact amount to be transferred before the transfer if the exact amount can be known in advance at the calling contract level, or set to an high enough amount (e.g. max uint) before the deposit and 0 immediately after, or just call the margin asset approve once when it is whitelisted using max uint but keep in mind that not all token contracts implements the 'unlimited approval' feature and therefore each transfer will decrease the allowance by the transferred amount.
The text was updated successfully, but these errors were encountered: