LockedBalance library should drop parameters to 96/32 bits

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/libraries/LockedBalance.sol#L56

Vulnerability details

Impact

The LockedBalance contract takes 256-bit amount values but performs bit math on them as if they were 96 bit values.
Bits could spill over to a different locked balance in the else part (lockedBalance stores two 128-bit locked balances in one 256-bit storage field):

function set(
  Lockups storage lockups,
  uint256 index,
  uint256 expiration,
  uint256 totalAmount
) internal {
  unchecked {
    // @audit-issue should drop totalAmount to 96, expiration to 128-96=32
    uint256 lockedBalanceBits = totalAmount | (expiration << 96);
    if (index % 2 == 0) {
      // set first 128 bits.
      index /= 2;
      // @audit-info clears upper bits, then sets them
      lockups.lockups[index] = (lockups.lockups[index] & last128BitsMask) | (lockedBalanceBits << 128);
    } else {
      // set last 128 bits.
      index /= 2;
      // @audit-info clears lower bits, then sets them
      // @audit-issue sets entire 256-bit lockedBalanceBits instead of just 128-bit
      lockups.lockups[index] = (lockups.lockups[index] & first128BitsMask) | lockedBalanceBits;
    }
  }
}

It could then increase the other, unrelated locked balance's amount leading to stealing funds from the protocol.
All callers of this function currently seem to ensure that totalAmount is indeed less than 96 bits but the LockedBalance library should be self-contained and not depend on the calling side to perform all checks.
If the code is ever extended and more calls to these functions are performed, it'll likely cause issues.

The same issue happens in setTotalAmount

Recommended Mitigation Steps

Make sure that there are only 96/32 bits set in totalAmount and expiration by dropping them to their respective types.

function set(
  Lockups storage lockups,
  uint256 index,
  uint256 expiration,
  uint256 totalAmount
) internal {
  unchecked {
-    uint256 lockedBalanceBits = totalAmount | (expiration << 96);
+    // cast it to uint256 again for the << 96 to work on 256-bits
+    uint256 lockedBalanceBits = uint256(uint96(totalAmount)) | (uint256(uint32(expiration)) << 96);
    
    ...
  }
}

[HardlyDifficult]

This is a good concern to flag. With this release we use a lot of slot packing strategies to try and save gas. We considered this suggestion, as well as potentially using the safe cast helpers from the OpenZeppelin library. For the moment at least, we decided not to make any changes here. We believe the assumptions behind these values are sound, and although it's mathematically possible for the assumptions to be violated, we don't believe it'll occur in production.

For totalAmount for example, that value is always bound by the amount of ETH a user has sent to the FETH contract. Here are some notes we have written up about the size assumptions in our codebase:

• ETH: uint96
  • Circulating supply is currently 119,440,269 ETH (119440269000000000000000000 wei / 1.2 * 10^26).
  • Max uint96 is 7.9 * 10^28.
  • Therefore any value capped by msg.value should never overflow uint96 assuming ETH total supply remains under 70,000,000,000 ETH.
    • There is currently ~2% inflation in ETH total supply (which fluctuates) and this value is expected to do down. We expect Ether will become deflationary before the supply reaches more than 500x current values.
  • 96 bits packs perfectly into a single slot with an address.
• Dates: uint32
  • Current date in seconds is 1643973923 (1.6 * 10^9).
  • Max uint32 is 4 * 10^9.
  • Dates will not overflow uint32 until 2104.
  • To ensure we don't behave unexpectedly in the future, we should require dates are <= max uint32.
  • uint40 would allow enough space for any date, but it's an awkward size to pack with.
• Sequence ID indexes: uint32
  • Our max sequence ID today is 149,819 auctions.
  • Max uint32 is 4 * 10^9.
  • Indexes will not overflow uint32 until we see >28,000x growth. This is the equiv to ~300 per block for every block in Ethereum to date.