Tigris Trade contest - eierina's results

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.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/utils/TradingLibrary.sol#L113-L114

Vulnerability details

Impact

Lack of validation on Chainlink price feeds may result in incorrectly functioning or non-functioning protocol.

For example:

• during high volatility a price feed may be suspended or become stale;
• on L2 networks the sequencer might be down
• on L2 networks the sequencer might be just restarted and a grace period should be waited for results to become reliable.

The results of using a deprecated API (as reported in my other issue opened related to Chainlink API) does not give a predictable response in these cases (may be 0, may be stale, may revert?).

Proof of Concept

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/utils/TradingLibrary.sol#L113-L114

            int256 assetChainlinkPriceInt = IPrice(_chainlinkFeed).latestAnswer();
            if (assetChainlinkPriceInt != 0) {
                ...
            }

Tools Used

Manual review

Recommended Mitigation Steps

• Use latestRoundData() API
• Use response's updatedAt (timestamp of when the round was updated) to verify the feed is not stale
• Refer to Chainlink documentation regarding sequencer handling with Arbitrum/Optimism L2 network