Foundation contest - cmichel's results

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/NFTMarketPrivateSale.sol#L166

Vulnerability details

Impact

The private sale signature used in buyFromPrivateSaleFor does not prevent replay attacks. If the NFT ends up in the original seller's wallet again (within the deadline) it can be purchased from them again.

POC

• S sells to B using a private sale. S creates a signature for it and B submits it and receives the NFT.
• The NFT market changes and the NFT is now worth more. (This can happen quickly, for example, if the NFT team announces new utility for the collection.) B lists it on some market for a higher price
• S wants the NFT again, buys it at the higher price
• B can resubmit the buyFromPrivateSaleFor(..., v, r, s) to buy it from S a second time at the lower initial price. S loses their NFT and B could list it for a higher price again.

Recommended Mitigation Steps

Use a nonce, indexed by (nftContract, tokenId), as part of the message to be signed. Increase the nonce on each buyFromPrivateSaleFor. Indexing the nonce by (nftContract, tokenId) ensures that each signature is only valid for a private sale of that NFT once.

- keccak256(abi.encode(BUY_FROM_PRIVATE_SALE_TYPEHASH, nftContract, tokenId, msg.sender, amount, deadline))
+ keccak256(abi.encode(BUY_FROM_PRIVATE_SALE_TYPEHASH, nftContract, tokenId, msg.sender, amount, deadline, nonces[nftContract][tokenId]++))

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/FETH.sol#L433

Vulnerability details

Impact

The FETH.withdrawFrom function does not validate its to parameter. Funds can be lost if to is the zero address.

Similar issues have been judged as medium recently, see Sandclock M-15 / Github issue

Recommended Mitigation Steps

Check that to != 0.

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);
    
    ...
  }
}

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/NFTMarketFees.sol#L78

Vulnerability details

Impact

The creator payouts are capped at MAX_ROYALTY_RECIPIENTS_INDEX. It's currently set to 4 and only 5 creators are paid out. Other creators are ignored.

Recommended Mitigation Steps

I don't think cases with more than 5 creators / royalty receivers are unlikely. It can and should probably be increased, especially as the transfers are already gas restricted.

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/NFTMarketPrivateSale.sol#L156

Vulnerability details

Impact

Similar to spoofing in finance, users can create private sales with correct signatures but then frontrun the buy with a transfer to a different wallet they control.

No funds are lost as this the NFT <> FETH exchange is atomic but it can be bad if third parties create a naive off-chain centralized NFT market based on this signature feature. It's also frustrating for the users if they try to accept the private sale but their transaction fails.

Recommended Mitigation Steps

This is made possible because private sales do not keep the NFT in escrow. Consider escrowing the NFT also for private sales.

QA Report

• FETH._deductAllowanceFrom does not emit an Approval event. It's not strictly required by EIP20 for transferFrom calls but without it off-chain scripts cannot correctly track the actual approvals. (It's also done in the OZ implementation)
• FETH._freeFromEscrow: comment should say: "if bucket NOT yet expired, that's the new start index"
• NFTMarketReserveAuction._transferFromEscrow: comment should say: "If the auction has ended, the highest bidder will be the new owner" as this reverts if auction has not ended
• NFTMarketFees: misleading naming. It's an amount, not a share as in creatorShares or totalShares. How about fee or royalty?
• NFTMarketOffer.makeOffer: The error should be "cannot be created" instead of "cannot be accepted" because this is an error in makeOffer, not in acceptOffer.