the `mint()` function in `CVX1.sol` can lead to loss of funds

[hats-bug-reporter[bot]]

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

Description:

Description

we can see that the mint() function transfers CVX tokens from the user to the CVX1 contract and then mints CVX1 but the issue is this function doesn't check the return value of the transfer, so if the transfer fails user able to mint CVX1 which is exchangeable with CVX token.

this scenario also can happen for withdraw() function. user will lose funds if transfer() fails in CVX because user CVX1 tokens will burn.

Impact

loss of funds for users who deposited CVX.

PoC

this is the mint function in CVX1

function mint(address receiver, uint256 amount) external {
        CVX.transferFrom(msg.sender, address(this), amount);

        _mint(receiver, amount);
    }

and this is the transfer function in CVX contract

function transfer(address recipient, uint256 amount) public virtual override returns (bool) {
        _transfer(_msgSender(), recipient, amount);
        return true;
    }

function _transfer(address sender, address recipient, uint256 amount) internal virtual {
        require(sender != address(0), "ERC20: transfer from the zero address");
        require(recipient != address(0), "ERC20: transfer to the zero address");

        _beforeTokenTransfer(sender, recipient, amount);

        _balances[sender] = _balances[sender].sub(amount, "ERC20: transfer amount exceeds balance");
        _balances[recipient] = _balances[recipient].add(amount);
        emit Transfer(sender, recipient, amount);
    }

we can see that the transfer() function in the CVX contract just returns bool value so the mint() function should revert when transfer fails.

Reccomendation

checks return value of transfer with bool or use OP safeTransfer.

[PlamenTSV]

When the transfer fails and reverts, the entire transaction for minting will revert due to the way the call chain works.

[0xdanial]

let's check again the transferFrom() function in the CVX contract.

function transferFrom(address sender, address recipient, uint256 amount) public virtual override returns (bool) {
        _transfer(sender, recipient, amount);
        _approve(sender, _msgSender(), _allowances[sender][_msgSender()].sub(amount, "ERC20: transfer amount exceeds allowance"));
        return true;
    }

    function _transfer(address sender, address recipient, uint256 amount) internal virtual {
        require(sender != address(0), "ERC20: transfer from the zero address");
        require(recipient != address(0), "ERC20: transfer to the zero address");

        _beforeTokenTransfer(sender, recipient, amount);

        _balances[sender] = _balances[sender].sub(amount, "ERC20: transfer amount exceeds balance");
        _balances[recipient] = _balances[recipient].add(amount);
        emit Transfer(sender, recipient, amount);
    }

we can see that when _transfer() fails the transferFrom() returns false so doesn't revert. in this case the mint() function in CVX1 contract should handles return value of transferFrom() function.

Checking the return value is a requirement, as written in the EIP-20 specification:

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

the severity of this issue should be medium because if it happens, the caller gains profit and other depositors lose funds.

consider this scenario:

• Bob calls mint() function and deposits 100 CVX token and get 100 CVX1 token
• Alice calls mint() function and transferFrom() got fails and Alice gets 100 CVX1 because returned value not handled.
• now Alice can call withdraw() function and get 100 CVX token for free.
• Bob lost funds because his deposited CVX tokens were withdrawn by Alice.

[PlamenTSV]

transferFrom internally calls _transfer which has 4 conditions to fail (technically they are 3):

1. Sender is address 0, not possible since you will be transferring them
2. The recipient is address 0, not possible since you are sending them to CVX1
3. If you try to send more tokens that you have in your account, the line: _balances[sender] = _balances[sender].sub(amount, "ERC20: transfer amount exceeds balance") will revert with the message "ERC20: transfer amount exceeds balance". The same revert would happen to the allowance.

The call chain is mint -> transferFrom -> _transfer -> revert if low balance, and the entire call chain would revert so no tokens get minted. Provide a PoC if I am missing something.

[0xdanial]

sorry for the late response

I couldn't find technically any condition where transferFrom() returns a false value but as I mentioned above as written in the EIP-20 specification, the caller must not assume the false value never returned. I think the possibility of this issue is rare but still can lead to loss of funds so severity can be low.

need to consider that might I miss something, if it's impossible for transferFrom() to return a false value why would return a true value at the end of the function?