Division round down 2 times may cause `convertToShares` calculation incorrect if underlying token with decimals less than 8.

[code423n4]

Lines of code

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

Vulnerability details

Impact

• In case fCash has not matured yet, convertToShares() may return incorrect value due to division round down 2 times. It may leads to the case that user need more amount of share than expected to withdraw assets.

• In wfCashERC4626.convertToShares() function, it calculated amout of share from assets amount. And it used a formula which round down the division (1st one).

  return (assets * totalSupply()) / totalAssets();

• But in totalAsset() function, it also rounded down 1 more time (2nd one) when fCash has not matured

  int256 pvExternal = (pvInternal * precision) / Constants.INTERNAL_TOKEN_PRECISION;

Proof Of Concept

1. Assume underlyingToken is USDC with decimals = 6 so precision = 1e6. Wrapped fCash has totalSupply() = 2000000, has not matured and pvInternal = 2111111. We try to get amount of share with assets = 1e9.

2. Use 2nd formula

   pvExternal = (pvInternal * precision) / INTERNAL_TOKEN_PRECISION
   = (2111111 * 1e6) / 1e8
   = 21111
   => totalAssets() = 21111

3. Use first formula, we get

   (assets * totalSupply()) / totalAssets()
   = (1e9 * 2000000) / 21111
   = 94737340722

4. But if we didn’t round down in first calculation, we will get

   (assets * totalSupply()) / totalAssets() 
   = (assets * totalSupply()) / ((pvInternal * precision) / INTERNAL_TOKEN_PRECISION)
   = (assets * totalSupply() * INTERNAL_TOKEN_PRECISION) / (pvInternal * precision)
   = (1e9 * 2000000 * 1e8) / (2111111 * 1e6)
   = 94736847091

Tools Used

Manual Review

Recommended Mitigation Steps

Merge 2 formula into 1 calculation to improve precision.

[jeffywu]

Good find and agree with the mitigation.

[jeffywu]

Duplicate of #155 (I think this is the better written description of all the duplicates).

I believe this should be Med Severity due to the size of the potential effect.

[jeffywu]

My understanding of the ERC4626 specification (and this comes via Alberto who is one of the authors of the specification), is that convertToShares and convertToAssets are intended to be oracle values that do not take into account slippage for an actual exit (that is the job of previewMint/previewWithdraw, etc). Therefore, convertToShares and convertToAssets must be resistant to flash loan manipulation (which they are here) and usable as view methods for UIs and display purposes, but should not be used by smart contracts to determine actual trading amounts.

If that is the case then I would think that the severity would be reduced for this particular report. I think that rounding in the previewMint/etc cases could be left as High.

 convertToShares

The amount of shares that the Vault would exchange for the amount of assets provided, in an ideal scenario where all the conditions are met.

MUST NOT be inclusive of any fees that are charged against assets in the Vault.

MUST NOT show any variations depending on the caller.

MUST NOT reflect slippage or other on-chain conditions, when performing the actual exchange.

MUST NOT revert unless due to integer overflow caused by an unreasonably large input.