ERC20 `approve` can fail for some tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Trading.sol#L652 https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Lock.sol#L117

Vulnerability details

ERC20 approve can fail for some tokens

Impact

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

Also approve() will fail for certain token implementations that do not return a boolean value. Hence it is recommend to use safeIncreaseAllowance() and safeDecreaseAllowance

Proof of Concept

• ERC20 token that can be for example USDT
• approve() return value ignored

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Trading.sol#L652 IERC20(_marginAsset).approve(_stableVault, type(uint).max);

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Lock.sol#L117 IERC20(assets[i]).approve(address(bondNFT), type(uint256).max);

Mitigation

• Add 0 approve before approve
• Consider using safeIncreaseAllowance and safeDecreaseAllowance instead of approve
• Check return value values and revert/emit events if needed

[c4-judge]

GalloDaSballo marked the issue as duplicate of #104

[c4-judge]

GalloDaSballo marked the issue as satisfactory