code-423n4 / 2022-06-notional-coop-findings

1 stars 1 forks source link

User can alter amount returned by redeem function due to control transfer #235

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L212-L222

Vulnerability details

Impact

Control is transferred to the receiver when receiving the ERC777. They are able to transfer the ERC777 to another account, at which time the before and after balance calculation will be incorrect.

        uint256 balanceBefore = IERC20(asset()).balanceOf(receiver);

        if (msg.sender != owner) {
            _spendAllowance(owner, msg.sender, shares);
        }
        _redeemInternal(shares, receiver, owner);
/////////////
Control is transferred to user. They can alter their balance here.
///////////

        uint256 balanceAfter = IERC20(asset()).balanceOf(receiver);
        uint256 assets = balanceAfter - balanceBefore;

//////////
Assets can be as low as 0 if they have transferred the same amount out as received.
//////////

        emit Withdraw(msg.sender, receiver, owner, assets, shares);
        return assets;

Tools Used

Manual review

jeffywu commented 2 years ago

Control of the contract is not transferred to anyone during a balanceOf call which is read only. No state can be modified.

fatherGoose1 commented 2 years ago

The control is transferred in _redeemInternal() which calls _burn() on the ERC777 which transfers control.

_burn() function logic here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/109778c17c7020618ea4e035efb9f0f9b82d43ca/contracts/token/ERC777/ERC777.sol#L390-L400

jeffywu commented 2 years ago

Understood, will change to confirmed