Notional x Index Coop - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashLogic.sol#L184

Vulnerability details

Impact

If maturity is reached and user has asked for redeem with opts.transferfCash as true, then if (hasMatured()) turns true at wfCashLogic.sol#L216 causing fcash to be cashed out in underlying token and then sent to receiver. So receiver obtains underlying when fcash was expected. The sender wont get an error thinking fcash transfer was success

Proof of Concept

1. User A calls redeem with opts.transferfCash as true and receiver as User B
2. Since maturity is reached so instead of transferring the fCash, contract would simply cash out fCash and sent the underlying token to the receiver which was not expected

Recommended Mitigation Steps

If opts.transferfCash is true and maturity is reached then throw an error mentioning that fCash can no longer be transferred

maxMarketIndex integrity is not verified in isValidMaturity function

Contract: https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/lib/DateTime.sol#L36

An invalid maxMarketIndex is simply accepted by isValidMaturity function. Any maturity compared against this incorrect maxMarketIndex will lead to incorrect maturity being declared valid.

Simply add below checks:

require(maxMarketIndex > 0, "CG: no markets listed");
require(maxMarketIndex <= Constants.MAX_TRADED_MARKET_INDEX, "CG: market index bound");

cashGroup.maxMarketIndex sanity is not verified

Contract: https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashBase.sol#L35

While initialization, provided maturity is checked against cashGroup obtained from NotionalV2.getCashGroup. But it is never checked that this cashGroup is actually lying within range of Constants.MAX_TRADED_MARKET_INDEX. If cashGroup.maxMarketIndex becomes greater than Constants.MAX_TRADED_MARKET_INDEX then this contract becomes useless as getMarketIndex will always fail.

Kindly add below check to verify sanity of cashGroup.maxMarketIndex

require(cashGroup.maxMarketIndex<=Constants.MAX_TRADED_MARKET_INDEX, "CG: market index bound");

No way to transfer dust funds

fCash is always computed in 8 decimals. However the underlying and asset tokens might be computed in higher decimal places. Since value will always be truncated to 8 decimal places, it might lead to loss of small dust funds which could accumulate over time with more mints over same token

Right now, there is no way to send this amount from contract to Admin. Consider having a function to sweep this values

Incorrect Market index can be derived

DateTime.getMarketIndex function is called with Constants.MAX_TRADED_MARKET_INDEX as maxMarketIndex, however this is not correct. Notional might not be allowing all 7 market index for a particular currency in which case cashGroup.maxMarketIndex will be lesser than Constants.MAX_TRADED_MARKET_INDEX.

This means getMarketIndex will check for non required market indexes as well and may show incorrect maturity market index

1. Assume currencyId is 1 and NotionalV2.getCashGroup(1) derives CashGroupSettings as cashGroup
2. Now for this cashGroup, cashGroup.maxMarketIndex is computed as 5
3. Now in current contract getMarketIndex function calls DateTime.getMarketIndex with Constants.MAX_TRADED_MARKET_INDEX instead of cashGroup.maxMarketIndex which means maturity is check against 7 indexes when it should have checked only for 5 indexes

Recommendation: Change function to

function getMarketIndex() public view override returns (uint8) {
		CashGroupSettings memory cashGroup = NotionalV2.getCashGroup(getCurrencyId());
		
        (uint256 marketIndex, bool isIdiosyncratic) = DateTime.getMarketIndex(
            cashGroup.maxMarketIndex,
            getMaturity(),
            block.timestamp
        );

        if (isIdiosyncratic) return 0;
        // Market index as defined does not overflow this conversion
        return uint8(marketIndex);
    }

https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashLogic.sol#L41

mintViaUnderlying function : if getMarketIndex() is 0 then there is no meaning in doing further processing and getting failed in called to Notional. This will be reverted as market is not tradeable at the moment. Better to have a check for this in beginning to prevent gas

require(getMarketIndex()!=0,"Non tradeable");

https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashLogic.sol#L41

Function mintViaUnderlying: If depositAmountExternal is submitted as 0 then there is no meaning in going further and failing at Notional ( causing more high gas). Add a check to revert beforehand

require(depositAmountExternal!=0, "Incorrect amount");

Use ++i instead of i++ to save gas