Upgraded Q -> H from 104 [1656255316696]

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#L42&gt;

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

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;
}  

The text was updated successfully, but these errors were encountered:

All reactions