Notional x Index Coop - kenzo's results

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L85 https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L21 https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L23 https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L71

Vulnerability details

EIP4626 states that maxWithdraw, convertToAssets, convertToShares, previewRedeem and previewWithdraw must not revert (unless due to large input, or due to a reason that will make deposit/redeem revert). However, wfCash4626's implementation of those ends up calling _getMaturedValue if the asset has matured. This call will revert if the account has not been settled yet.

Impact

Incorrect implementation of EIP4626. Protocols that will build upon wfCash4626 may revert unexpectedly.

Proof of Concept

All these functions end up calling _getMaturedValue via totalAssets. getMaturedValue will revert if the account has not been settled yet:

        // We cannot settle an account in a view method, so this may fail if the account has not been settled
        // after maturity. This can be done by anyone so it should not be an issue
        (int256 cashBalance, /* */, /* */) = NotionalV2.getAccountBalance(currencyId, address(this));
        int256 underlyingExternal = NotionalV2.convertCashBalanceToExternal(currencyId, cashBalance, true);
        require(underlyingExternal > 0, "Must Settle");

Therefore, the functions don't implement EIP4626 correctly, and integration with wfCash might fail.

Recommended Mitigation Steps

Create a view method that will simulate the settlement of an account and get the cash balance of the account's currencyId. Then use that cashBalance to calculate the underlyingExternal.

Lines of code

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

Vulnerability details

withdraw will revert if the account has not been settled yet. This is just due to the implementation and can be avoided by, well, settling the account.

Impact

withdraw reverts unnecessarily. Protocols and users which will use wfCash4626 will have to discover this and settle by themselves.

Proof of Concept

withdraw calls previewWithdraw, which ends up calling _getMaturedValue, which will revert if the account is not settled yet.

Recommended Mitigation Steps

Add to withdraw:

NotionalV2.settleAccount(address(this));

This will ensure that the account is settled and withdraw will not revert.