code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

Calling approve() without first calling approve(0) will revert with some tokens, such as Tether (USDT). While Tether is known to do this, it applies to other tokens as well, which are trying to protect against this attack vector. In this prior issue the judge upgraded the finding to a risk of Medium. In addition, the code does not check the return code of the approve() call. In discord, the sponsor mentions that There could be other stables added, however, USDT will definitely not be added ... it's the sketchiest of all centralized stablecoins, it always has FUD and they hold debt in chinese real estate development firms which is a very suspect sector because of the recent defaults. ... Any stablecoin that is verifiably fully backed or overcollateralized and offers access to an attractive DeFi yield source could be a candidate and these caveats were not mentioned in the README.md as part of the scope. Even granting the sponsor this carve-out, while Tether is currently excluded, there is nothing stopping it from being fully backed in the future, or some other fully-backed token with this issue being added. If the sponsor adds more caveats after I've submitted, please verify timestamps of messages compared to my submission's timestamp.

Proof of Concept

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

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

Tools Used

Code inspection

Recommended Mitigation Steps

Use OpenZeppelin’s SafeERC20's safeApprove()/safeIncreaseAllowance()

ElliotFriedman commented 2 years ago

USDT or other non-standard ERC20 tokens will not be allowed into the PCV ever so this issue is invalid.