Withdraw function does not conform to EIP4626

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L186-L191

Vulnerability details

Impact

The withdraw of wfCashERC4626 is not 4626 compatible. wfCashERC4626.sol#L186-L191

According to EIP4626

Burns shares from owner and sends exactly assets of underlying tokens to receiver.

The withdraw function of ERC4626 should send the exact same amount of assets according to the ERC4626. However, the fCash contract would send fewer assets to the receiver. This may lead to a loss of user funds on other protocols. In certain cases, it may cause an exploit if leads to an underflow.

Here's a possible adoption scenario:

A yield aggregator with default vault set to wfcash with the following deposit function.

function withdraw(uint256 amount, address receiver) external returns(uint256 share){
    uint256 burnShares = defaultVault.withdraw(amount, address(this));
    _burn(msg.sender, burnShares);
    token.safeTransfer(msg.sender, amount);
}

In the above scenario, the yield aggregator vault can not withdraw funds.

Proof of Concept

dai.functions.approve(wrapper.address, deposit_amount).transact()
mint_fcash_amount = 100 * 10**8
prev_dai = dai.functions.balanceOf(user).call()
wrapper.functions.withdraw(10**18, user, user).transact()
current_dai = dai.functions.balanceOf(user).call()
print(current_dai - prev_dai)

999999999840333625

Tools Used

Manual inspection.

Recommended Mitigation Steps

A better solution is to make some modifications to the notional protocol to support this functionality. If that's not doable, there's a more common solution. The vault should withdraw slightly more fcash and deposit back to the notional protocol.

Both withdraw and deposit functions do not conform to EIP-4626 which may lead to a loss of funds.

Further, there are several small nuances of EIP-4626 that make wfcash not compatible to ERC4626. (Will submit a QA report later) Little nuances matter. Take, USDT, for example. A lot of gas was wasted in order to support USDT in DEFI protocols.

I admire the team that want to make wfcash as good as possible, however, a wrapped token is different to a vault token. It's difficult to achieve two things at the same time IMHO. I recommend the team to re-consider whether to make wfcash 4626 compatible.

[jeffywu]

Duplicate #143