search

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

1 stars 1 forks source link

wfCashERC4626 maxWithdraw, previewWithdraw, previewRedeem, convertToAssets, convertToShares doesn't conform to EIP4626 #167

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

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.

berndartmueller commented 2 years ago

Duplicate of #215 - [L-09] wfCashERC4626 contract does not conform to EIP4626

(Disclaimer: That's my QA report)

I explicitly did not submit this finding as medium severity as I thought it does not qualify:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

jeffywu commented 2 years ago

Agree that this can be a QA report, there is no way for this to result in loss of funds.

gzeoneth commented 2 years ago

This might be controversial but I consider this as a duplicate of #155. EIP4626 is aimed to create a consistent and robust implementation patterns for Tokenized Vaults. A slight deviation from 4626 would broke composability and potentially lead to loss of fund (POC in #88 can be an example). It is counterproductive to implement EIP4626 but does not conform to it fully.