code423n4 commented 1 year ago

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:

/**
* @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender.
* @param _spender The address which will spend the funds.
* @param _value The amount of tokens to be spent.
*/
function approve(address _spender, uint _value) public onlyPayloadSize(2 * 32) {

    // To change the approve amount you first have to reduce the addresses`
    //  allowance to zero by calling `approve(_spender, 0)` if it is not
    //  already 0 to mitigate the race condition described here:
    //  https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
    require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

    allowed[msg.sender][_spender] = _value;
    Approval(msg.sender, _spender, _value);
}

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.

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #104