Notional x Index Coop - sseefried's results

QA Report

Remarks/Recommendations

• The idea behind a 6/30/90/360 measure for WEEK/MONTH/QUARTER/YEAR was really interesting. But it's also misleading. Perhaps call them "Notional Weeks", "Notional Months". Thinking of a month as being 30 days and a year as 360 days is not such a stretch but a 6 day week is 14.3% different from reality.

  An alternative might be 7 day weeks, 28 day "months", 13 months a year, and 1 special day -- New Years Day -- that is part of no month. (In a leap year have two such special days.) 7*4*13 + 1 = 365.

Typos

wfCashBase.sol:97-99

• "idd" -> "id"
• getDecodedID -> getDecodedId (to be consistent with getCurrencyId)

WrappedfCashFactory.sol:30

• "it's" -> "its"

Non-critical: Don't use magic numbers

The magic numbers used in functions to encode trades, e.g. EncodeDecode.sol:76-79 are error prone.

Perhaps define some global constants near the definition of enum TradeActionType (Types.sol:18-34) as follows:

uint256 constant LendMinImpliedRateOffset = 120;
uint256 constant LendFCashAmountOffset = LendMinImpliedRateOffset + 32;
uint256 constant LendMarketIndexOffset = LendFCashAmountOffset + 88;
uint256 constant LendTradeActionTypeOffset = LendMarketIndex + 8;

and then rewrite the relevant part as follows:

action[0].trades[0] = bytes32(
    (uint256(uint8(TradeActionType.Borrow)) << LendTradeActionTypeOffset) |
    (uint256(marketIndex) << LendMarketIndexOffset) |
    (uint256(fCashAmount) << LendFCashAmountOffset) |
    (uint256(maxImpliedRate) << LendMinImpliedRateOffset)

Non-critical: Inconsistent use of maxImpliedRate

Struct RedeemOpts says a maxImpliedRate of zero means no limit but NotionalTradeModule._redeem uses type(uint32).max.

Non-critical: Improve comments in wfCashERC4626.deposit and wfCashERC4626.mint

Change the comment Will revert if matured to Will revert if matured or has idiosyncratic maturity since it will also revert under those circumstances.

Low Risk: wfCashERC4626._safeNegInt88 is not safe

Links to affected code

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

Impact

function _safeNegInt88(uint256 x) private pure returns (int88) {
    int256 y = -int256(x);
    require(int256(type(int88).min) <= y);
    return int88(y);
}

Because of the way Two's Complement arithmetic works this function will actually return positive values for x >= 2**255 + 2*87 + 1.

This becomes more clear when expressed in hexadecimal: x >= 0x8000000000000000000000000000000000000000008000000000000000000001.

In fact, this particular value is the largest positive value that could be returned from _safeNegInt88 as it is currently written.

Looking back through commits to the original (non-competition) repo we see that the function was used in functions convertToAssets and convertToShares which would have been a problem but is no longer.

Proof of Concept

Instead of focusing on types we will talk purely in terms of bit-patterns -- expressed in hexadecimal for brevity -- throughout this PoC to keep things clear.

• Let x have bit-pattern 0x8000000000000000000000000000000000000000008000000000000000000001.
• int256(x) has the same bit-pattern
• -int256(x) flips all the bits and adds one. Thus y has bit-pattern 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF7FFFFFFFFFFFFFFFFFFFFF.
• This is interpreted as a positive number as the sign-bit is 0.
• int256(type(int88).min) is a negative number so
• int256(type(int88).min) <= y
• Finally int88(y) will keep the sign-bit and discard the next 255 - 87 bits giving us the bit-pattern of 7FFFFFFFFFFFFFFFFFFFFF.
• This is interpreted as 2**87 - 1 in Two's Complement arithmetic.

Many different values can produce this same value. All that is required is that the first bit of the number is 1 and the final 88 bits have bit-pattern 0x8000000000000000000001

e.g. 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF8000000000000000000001 will produce the same output as will 0xCAFEBABECAFEBABECAFEBABECAFEBABECAFEBABEEE8000000000000000000001

Tools Used

Manual Inspection

Recommended Mitigation Steps

Assuming the intention of the function was to return the negative of any number up to 2**87 - 1 it should be written as:

function _safeNegInt88(uint256 x) private pure returns (int88) {
  require(x < 2**87, "too big!");
  return -int88(int256(x));
}