transfer return value is not checked, use safeTransfer() instead

[hats-bug-reporter[bot]]

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

Description: Affected code https://github.com/Cvg-Finance/hats-audit/blob/f581ec0db856d58a4b0063b9dae7ef65774a846b/contracts/Staking/Convex/cvgCVX/CvxConvergenceLocker.sol#L268

Description In CvxConvergenceLocker.sol contract, sendTokens() function is called by owner of contract to send the tokens to receiver address. These tokens are actually received by CvxConvergenceLocker contract so intead of permanently locking it, the protocol has introduced this function is contract so that it can be sent to recipient address. sendTokens() restricts the transfer of CVX, CRV, FXS, cvgCVX token as these are reward tokens. For information, the contracts will be deployed on Ethereum mainnet so the issue is relevant to Ethereum mainnet as detaile below:

    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]);       @audit // has used  unsafe transfer method
            unchecked {
                ++i;
            }
        }
    }

This tokens received by CvxConvergenceLocker contract can be any token. It can be USDT, USDC or any token. This issue is relevant to tokens like USDT, BNB, OMG, etc as these tokens do not check return value on its transfers.

As can be seen in sendTokens(), It has used unsafe transfer() method to transfer the tokens to receiver address.

The issue here is with the use of unsafe transfer() function. The ERC20.transfer() function return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.

Some tokens (like USDT or similar tokens) don't correctly implement the EIP20 standard and their transfer() function return void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert. Please check USDT transfer() implementation, it does not return success.

In sendTokens(), tokens are being tranferred to receiver address and return value is not checked. This could lead to silent failure of transfer. This can cause loss of user/protocol funds.

Impact Tokens that don't actually perform the transfer and return false are still counted as a correct transfer and tokens that don't correctly implement the latest EIP20 spec will be unusable in the protocol as they revert the transaction because of the missing return value. This will lead to loss of token transfers. The CvxConvergenceLocker contracts specifically wants to transfer such tokens to recipient with the exeception to restrict reward tokens transfer to receiver address. Due to silent failure of transfer, there could be loss of tokens and this function is one of the functionality of protocol which is getting affected by this issue so Medium severity is identified per contest rules.

Recommendation to fix The contract has already used openzeppelin's safeERC20.sol so recommend to use safeTransfer() function instead of transfer(). Any weird behaviour of tokens present here like revert on zero transfer and so on.. fix the issue with below recommendation.

    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]);
+            tokens[i].safeTransfer(receiver, amounts[i]);
            unchecked {
                ++i;
            }
        }
    }

[PlamenTSV]

2

[0xRizwan]

@PlamenTSV

First of all, #2 is invalid based on my reasoning here and it is not a duplicate of #2

Based on your comment on #2,

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.

This highlighted issue is only relevant for Ethereum mainnet where the contracts will be deployed. sendTokens() is specifically added in contract to recover the tokens except reward tokens so any such tokens can not be considered as OOS. The issue is not about transfer blacklisting but more focused on transfer functions return value.

For example: transfer() function of USDT violates EIP20 and it is implemented as:

    function transfer(address _to, uint _value) public onlyPayloadSize(2 * 32) {
        uint fee = (_value.mul(basisPointsRate)).div(10000);
        if (fee > maximumFee) {
            fee = maximumFee;
        }
        uint sendAmount = _value.sub(fee);
        balances[msg.sender] = balances[msg.sender].sub(_value);
        balances[_to] = balances[_to].add(sendAmount);
        if (fee > 0) {
            balances[owner] = balances[owner].add(fee);
            Transfer(msg.sender, owner, fee);
        }
        Transfer(msg.sender, _to, sendAmount);
    }

USDT transfer does not check return value so it can fail silently without reverts. Per EIP20, transfer() function must check return value:

 function transfer(address _to, uint256 _value) public returns (bool success)

Such issues has historically judged as Medium severity on all major public audit platforms.

@PlamenTSV I agree with your point on it does not break the core contract functionality so per contest rules, it does not fit in Medium severity.

I think, This is low severity issue and should be considered due tokens weird behaviour on Ethereum mainnet.

[PlamenTSV]

Judging by the same logic, I can make any ERC20 with incompliant logic and send it to the contract. Also the sendTokens() is owner managed, any reason for the transfer to revert would be because of the owner with the conditions being he provides wrong amounts. You cannot account for every token in existence inside a quality of life function.

[0xRizwan]

@PlamenTSV

First of all, #2 is invalid based on my reasoning here and it is not a duplicate of #2

Based on your comment on #2,

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.

This highlighted issue is only relevant for Ethereum mainnet where the contracts will be deployed. sendTokens() is specifically added in contract to recover the tokens except reward tokens so any such tokens can not be considered as OOS. The issue is not about transfer blacklisting but more focused on transfer functions return value.

For example: transfer() function of USDT violates EIP20 and it is implemented as:

    function transfer(address _to, uint _value) public onlyPayloadSize(2 * 32) {
        uint fee = (_value.mul(basisPointsRate)).div(10000);
        if (fee > maximumFee) {
            fee = maximumFee;
        }
        uint sendAmount = _value.sub(fee);
        balances[msg.sender] = balances[msg.sender].sub(_value);
        balances[_to] = balances[_to].add(sendAmount);
        if (fee > 0) {
            balances[owner] = balances[owner].add(fee);
            Transfer(msg.sender, owner, fee);
        }
        Transfer(msg.sender, _to, sendAmount);
    }

USDT transfer does not check return value so it can fail silently without reverts. Per EIP20, transfer() function must check return value:

 function transfer(address _to, uint256 _value) public returns (bool success)

Such issues has historically judged as Medium severity on all major public audit platforms.

@PlamenTSV I agree with your point on it does not break the core contract functionality so per contest rules, it does not fit in Medium severity.

I think, This is low severity issue and should be considered due tokens weird behaviour on Ethereum mainnet.

@0xR3vert I kindly seek your response on this issues. Historically, such issues are Medium severity at sherlock/code4rena/cantina. Based on contest rules, i believe this issue still deserves low severity.