search

code-423n4 / 2022-03-volt-findings

0 stars 0 forks source link

`.approve()` might fail for USDT in case of multiple deposits #7

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/pcv/compound/ERC20CompoundPCVDeposit.sol#L31

Vulnerability details

Impact

USDT approvals work differently than the other ERC20 contracts. For USDT, .approve() function cannot be called for updating allowances from a non-zero value to a non-zero value.

Proof of Concept

This instance in the code is trying to change the allowance to a non-zero value but in case of multiple deposits for USDT token, the approval will fail and will cause the following operations to break.

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/pcv/compound/ERC20CompoundPCVDeposit.sol#L31

Tools Used

Manual checks

Recommended Mitigation Steps

Instead of calling the .approve() function directly, a second line of code can be added which sets the approval to zero first and then sets it back to the desired amount. For eg:

token.approve(address(cToken), 0);
token.approve(address(cToken), amount);

Another alternative would be to use 'increaseAllowanceanddecreaseAllowance` from the ERC20 contracts.

ElliotFriedman commented 2 years ago

This would be an issue if we were using USDT, however, the VOLT system will likely never hold USDT in the PCV, so this is not a risk.

ElliotFriedman commented 2 years ago

duplicate https://github.com/code-423n4/2022-03-volt-findings/issues/45

ElliotFriedman commented 2 years ago

duplicate https://github.com/code-423n4/2022-03-volt-findings/issues/1