Use `safeTransfer` instead of `transfer`.

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xde40d71e2748475f15931cbe34b1b9e6e1a22333bafcd9eafafab028eb0d655c Severity: medium

Description: Description\ sendTokens is a function that is used to transfer tokens that were (in most cases) incorrectly sent to the contract.

function sendTokens(IERC20[] calldata tokens, uint256[] calldata amounts, address receiver) external onlyOwner {
        require(tokens.length == amounts.length, "LENGTH_MISMATCH");

        for (uint256 i; i < tokens.length; ) {
            /// @dev Ensure token is not one from the below list (CVX, CRV, FXS, cvgCVX) as these are reward tokens
            require(address(tokens[i]) != address(CVX), "CVX_CANNOT_BE_TRANSFERRED");
            require(address(tokens[i]) != address(CRV), "CRV_CANNOT_BE_TRANSFERRED");
            require(address(tokens[i]) != address(FXS), "FXS_CANNOT_BE_TRANSFERRED");
            require(address(tokens[i]) != address(this), "CVGCVX_CANNOT_BE_TRANSFERRED");

            tokens[i].transfer(receiver, amounts[i]);
            unchecked {
                ++i;
            }
        }
    }

You can see that the token can be any token, except CVX, CRV, FXS and CVGCVX, as these are reward tokens.

Because tokens can be realistically any token, it's important to note that there are some tokens that do not revert on failure, but instead, they return false. EIP-20 also confirms this and specifically states that contracts should check the return value of both transfer and transferFrom in order to handle the transfer properly.

Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

Currently, this doesn't happen as the return value of transfer isn't checked.

tokens[i].transfer(receiver, amounts[i]);

Because of this, the transfer might fail, but the contract will be non the wiser and will continue executing.

The issue occurs in CVX1.sol#recoverRewards() as well.

Attachments

1. Proof of Concept (PoC) File PoC in comments

2. Revised Code File (Optional) Use OZ's SafeERC20

[0xRizwan]

I have submitted #4 and i believe this issue is invalid with below reasons:

1) The issue described is highly speculative and it does not mention that this issue is only relevant for Ethereum mainnet. Most of the chains including L2 does not have this issue related to unsafe transfer method. The issue fails to describe why, where and how the issue will impact the current implementation.

2) The tokens which shows this weird behaviour and non-compliance of EIP20 is not mentioned in description. Mere any token consideration wont make it a valid issue as only few tokens like USDT, etc shows non standard behaviour only on Ethereum mainnet.

I would let the sponsor decide on validation of both issues.

[PlamenTSV]

I believe such tokens are OOS and even so, transfer failures do not lead to any bricking of core functionality. You could argue for USDT, but the only reason the transfer would fail is if the receiver is block-listed. In that case just change the receiver and the transfer will succeed.