Notional x Index Coop - minhquanym's results

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.

 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.

1. Unused private function _safeNegInt88() in wfCashERC4626

Impact

In wfCashERC4626 contract, function _safeNegInt88() is declared but never use. And it’s a private function, so no one can call it outside the contract either.

Proof Of Concept

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

Recommended Mitigation Steps

Remove _safeNegInt88() function.

2. Call totalSupply() 2 times to get already cached value.

Impact

In wfCachERC4626.convertToShares(), line 53 has already cached totalSupply but line 60 call totalSupply() again instead of using supply variable directly. It’s inconsistent when compare to convertToAssets() function which use supply variable.

Proof Of Concept

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

Recommended Mitigation Steps

Use supply instead of totalSupply()

3. Implementation is not align with documentation

Detail

Function NotionalTradeModule._mintFCashPosition() is used to mint fCash position, but the natspec docs said that this function is used to redeem fCash position

Proof of concept

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L418

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L415

4. Inconsistent use of floating pragma and fixed solidiy version

Impact

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

https://swcregistry.io/docs/SWC-103

In wrapped fcash, it uses ^0.8.0 and 0.8.11 inconsistently

Occurences

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

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

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

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

Recommended Mitigation Steps

Use fixed solidity version

1. Save gas by stored traded market in array in DateTime.sol

Impact

In DateTime.getTradedMarket() function, it used 7 if to get result which mean it’s cost at most 7 check every time this function is called.

In DateTime.getMarketIndex() function, it loop from 1 to maxMarketIndex and call getTradedMarket() each time. Instead if we stored result of getTradedMarket() in a list like below we can save gas (at most 7*7 = 49 checks)

uint256[] list = [QUARTER, 2 * QUARTER, YEAR, 2 * YEAR, … ]

Proof of concept

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

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

Recommended Mitigation Steps

Precompute and store result of getTradedMarket() in a list