Upgraded Q -> H from 104 [1656255316696]

[code423n4]

Judge has assessed an item in Issue #104 as High risk. The relevant finding follows:

L02: Incompatibility with ERC-4626

Line References

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

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

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

Description

The EIP-4626 specification requires that totalAssets() to NOT revert, but the current implementation does so in the underlying methods:

int256 underlyingExternal = NotionalV2.convertCashBalanceToExternal(currencyId, cashBalance, true);
require(underlyingExternal > 0, "Must Settle");

int256 pvExternal = (pvInternal * precision) / Constants.INTERNAL_TOKEN_PRECISION;
// PV should always be >= 0 since we are lending
require(pvExternal >= 0);

Recommended Mitigation Steps

Consider returning 0 if the condition is not met.

if (underlyingExternal < 0) return 0;

if (pvExternal < 0) return 0;

As a consequence, because totalAssets() can now possibly return 0, convertToShares() has to be modified to check that totalAssets() is non-zero instead of totalSupply() to ensure compatibility with the specification: “MUST NOT revert unless due to integer overflow caused by an unreasonably large input.”

function convertToShares(uint256 assets) public view override returns (uint256 shares) {
  uint256 totalAssets = totalAssets();
  if (totalAssets == 0) {
    // Scales assets by the value of a single unit of fCash
    uint256 unitfCashValue = _getPresentValue(uint256(Constants.INTERNAL_TOKEN_PRECISION));
    return (assets * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / unitfCashValue;
  }
  return (assets * totalSupply()) / totalAssets;
}

[gzeoneth]

Duplicate of https://github.com/code-423n4/2022-06-notional-coop-findings/issues/155

[jeffywu]

@gzeoneth I do not believe this is a duplicate of #155, although it also relates to ERC4626 compatibility. It is a separate issue regarding potential integer underflows in a separate part of the implementation.

[gzeoneth]

I have commented in #155

Judging this and all duplicate regarding EIP4626 implementation as High Risk. 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 https://github.com/code-423n4/2022-06-notional-coop-findings/issues/88 can be an example). It is counterproductive to implement EIP4626 but does not conform to it fully. Especially it does seems that most of the time deposit would be successful but not withdraw, making it even more dangerous when an immutable consumer application mistakenly used the wfcash contract.

I don't believe each of these issues (although in different part of the contract) deserves a unique High Risk finding because individually none of them lead to direct loss of fund. I think the bigger picture is that general incompatibility with EIP4626 may lead to loss of fund with integration that expected compliance with EIP4626, and that's why I consider them all as duplicates.

[gzeoneth]

Alternatively I can judge each of them as a Med RIsk issue, but I would prefer to stress the High Risk nature of EIP4626 non-compliance. In terms of warden reward a High Risk with 5 duplicate should be similar to solo Med Risk.

[jeffywu]

Understood, I don't see any need to change the judging in that case.