Foundation contest - IllIllI's results

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/a03a7e198c1dfffb1021c0e8ec91ba4194b8aa12/contracts/mixins/NFTMarketCreators.sol#L158-L160 https://github.com/code-423n4/2022-02-foundation/blob/a03a7e198c1dfffb1021c0e8ec91ba4194b8aa12/contracts/mixins/NFTMarketCreators.sol#L196-L198 https://github.com/code-423n4/2022-02-foundation/blob/a03a7e198c1dfffb1021c0e8ec91ba4194b8aa12/contracts/mixins/NFTMarketCreators.sol#L97-L99

Vulnerability details

According to the README.md

All sales in the Foundation market will pay the creator 10% royalties on secondary sales. This is not specific to NFTs minted on Foundation, it should work for any NFT. If royalty information was not defined when the NFT was originally deployed, it may be added using the Royalty Registry which will be respected by our market contract.

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/README.md?plain=1#L21

Using the Royalty Registry an owner can decide to change the royalty information right before the sale is complete, affecting who gets what.

Impact

By updating the registry to include the seller as one of the royalty recipients, the creator can steal the sale price minus fees. This is because if code finds that the seller is a royalty recipient the royalties are all passed to the creator regardless of whether the owner is the seller or not.

Proof of Concept

          // 4th priority: getRoyalties override
          if (recipients.length == 0 && nftContract.supportsERC165Interface(type(IGetRoyalties).interfaceId)) {
            try IGetRoyalties(nftContract).getRoyalties{ gas: READ_ONLY_GAS_LIMIT }(tokenId) returns (
              address payable[] memory _recipients,
              uint256[] memory recipientBasisPoints
            ) {
              if (_recipients.length > 0 && _recipients.length == recipientBasisPoints.length) {
                bool hasRecipient;
                for (uint256 i = 0; i < _recipients.length; ++i) {
                  if (_recipients[i] != address(0)) {
                    hasRecipient = true;
                    if (_recipients[i] == seller) {
                      return (_recipients, recipientBasisPoints, true);

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L127-L154

When true is returned as the final return value above, the following code leaves ownerRev as zero because isCreator is true

      uint256 ownerRev
    )
  {
    bool isCreator;
    (creatorRecipients, creatorShares, isCreator) = _getCreatorPaymentInfo(nftContract, tokenId, seller);

    // Calculate the Foundation fee
    uint256 fee;
    if (isCreator && !_nftContractToTokenIdToFirstSaleCompleted[nftContract][tokenId]) {
      fee = PRIMARY_FOUNDATION_FEE_BASIS_POINTS;
    } else {
      fee = SECONDARY_FOUNDATION_FEE_BASIS_POINTS;
    }

    foundationFee = (price * fee) / BASIS_POINTS;

    if (creatorRecipients.length > 0) {
      if (isCreator) {
        // When sold by the creator, all revenue is split if applicable.
        creatorRev = price - foundationFee;
      } else {
        // Rounding favors the owner first, then creator, and foundation last.
        creatorRev = (price * CREATOR_ROYALTY_BASIS_POINTS) / BASIS_POINTS;
        ownerRevTo = seller;
        ownerRev = price - foundationFee - creatorRev;
      }
    } else {
      // No royalty recipients found.
      ownerRevTo = seller;
      ownerRev = price - foundationFee;
    }
  }

In addition, if the index of the seller in _recipients is greater than MAX_ROYALTY_RECIPIENTS_INDEX, then the seller is omitted from the calculation and gets zero (_sendValueWithFallbackWithdraw() doesn't complain when it sends zero)

        uint256 maxCreatorIndex = creatorRecipients.length - 1;
        if (maxCreatorIndex > MAX_ROYALTY_RECIPIENTS_INDEX) {
          maxCreatorIndex = MAX_ROYALTY_RECIPIENTS_INDEX;
        }

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

This issue does a lot of damage because the creator can choose whether and when to apply it on a sale-by-sale basis. Two other similar, but separate, exploits are available for the other blocks in _getCreatorPaymentInfo() that return arrays but they either require a malicious NFT implementation or can only specify a static seller for which this will affect things. In all cases, not only may the seller get zero dollars for the sale, but they'll potentially owe a lot of taxes based on the 'sale' price. The attacker may or may not be the creator - creators can be bribed with kickbacks.

Tools Used

Code inspection

Recommended Mitigation Steps

Always calculate owner/seller revenue separately from royalty revenue

    bool isCreator = false;
    // lookup for tokenCreator
    try ITokenCreator(nftContract).tokenCreator{ gas: READ_ONLY_GAS_LIMIT }(tokenId) returns (
      address payable _creator
    ) {
      isCreator = _creator == seller;
    } catch // solhint-disable-next-line no-empty-blocks
    {
      // Fall through
    }

    (creatorRecipients, creatorShares) = _getCreatorPaymentInfo(nftContract, tokenId);

Lines of code

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

Vulnerability details

According to the README.md

If royalty information was not defined when the NFT was originally deployed, it may be added using the Royalty Registry which will be respected by our market contract.

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/README.md?plain=1#L21

The actual exchange code only respects the Royalty Registry or other royalty information if the number of recipients is less than or equal to four.

Impact

If the creatorRecipients.length is more than four then the array is essentially truncated and the royalties are only split among the first four entries in the array. If the array happens to be sorted from low to high then the people who were supposed to get the largest portions of the royalties are given nothing.

Proof of Concept

        uint256 maxCreatorIndex = creatorRecipients.length - 1;
        if (maxCreatorIndex > MAX_ROYALTY_RECIPIENTS_INDEX) {
          maxCreatorIndex = MAX_ROYALTY_RECIPIENTS_INDEX;
        }

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

        // Send payouts to each additional recipient if more than 1 was defined
        uint256 totalDistributed;
        for (uint256 i = 1; i <= maxCreatorIndex; ++i) {
          uint256 share = (creatorFee * creatorShares[i]) / totalShares;
          totalDistributed += share;
          _sendValueWithFallbackWithdraw(creatorRecipients[i], share, SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS);
        }

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

Creators shouldn't have to settle the correct amounts amongst themselves afterwards and doing so may trigger unwanted tax consequences for the creators who got the larger shares of funds.

Tools Used

Code inspection

Recommended Mitigation Steps

Fetch the royalty information during offer creation, cache it for the final transfer, and reject any NFT for which the array size is more than MAX_ROYALTY_RECIPIENTS_INDEX

There are many places in the code where it is assumed that variables will never overflow, but just stating the assumption does not protect users from the loss of funds. There are many people (e.g. bitcoin maximalists) who believe that all other coins will inflate away to zero eventually so it's important for code to be written to handle such scenarios. Code that has underflow/overflow protection will reject the addition of new data, whereas the sponsor's code will lose users' money if that ever happens.

Using unchecked {} for assumptions rather than certainties is not the correct use of the keyword. This is an example of the sponsor's code where unchecked {} is used correctly:

    if (msg.value < amount) {
      // The check above prevents underflow with delta.
      unchecked {
        uint256 delta = amount - msg.value;
        if (accountInfo.freedBalance < delta) {
          revert FETH_Insufficient_Available_Funds(accountInfo.freedBalance);
        }
        // The check above prevents underflow of freed balance.
        accountInfo.freedBalance -= uint96(delta);
      }

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

Given that delta is less than freedBalance which is a uint96, it's mathematically impossible for freedBalance to underflow.

Impact

The following are examples of code where overflow/underflow are mathematically possible, and when such events occur the user will lose his/her funds/NFTs

Proof of Concept

      unchecked {
        pendingWithdrawals[user] += amount;
      }

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

    unchecked {
      accountInfo.freedBalance += uint96(msg.value);
    }

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

    unchecked {
      toAccountInfo.freedBalance += uint96(amount);
    }

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

      unchecked {
        accountInfo.allowance[msg.sender] -= amount;
      }

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

    unchecked {
      accountInfo.freedBalance -= uint96(amount);
    }

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

      unchecked {
        accountInfo.freedBalance += escrow.totalAmount;
        accountInfo.lockups.del(escrowIndex);
        // Escrow index cannot overflow 32 bits.
        escrow = accountInfo.lockups.get(escrowIndex + 1);
      }

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

      unchecked {
        // Increment the escrow start index if the next bucket is not empty
        ++escrowIndex;
      }

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

    unchecked {
      accountInfo.lockupStartIndex = uint32(escrowIndex);
    }

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

    unchecked {
      // The number of buckets is always < 256 bits.
      for (uint256 escrowIndex = accountInfo.lockupStartIndex; ; ++escrowIndex) {
        LockedBalance.Lockup memory escrow = accountInfo.lockups.get(escrowIndex);
        if (escrow.expiration == 0) {
          if (expiration > type(uint32).max) {
            revert FETH_Expiration_Too_Far_In_Future();
          }
          // Amount (ETH) will always be < 96 bits.
          accountInfo.lockups.set(escrowIndex, expiration, amount);
          break;
        }
        if (escrow.expiration == expiration) {
          // Total ETH will always be < 96 bits.
          accountInfo.lockups.setTotalAmount(escrowIndex, escrow.totalAmount + amount);
          break;
        }
      }

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

    unchecked {
      accountInfo.freedBalance += uint96(amount);
    }

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

          unchecked {
            ++accountInfo.lockupStartIndex;
          }

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

        unchecked {
          ++escrowIndex;
        }

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

Tools Used

Code inspection

Recommended Mitigation Steps

Don't use unchecked {} unless it's mathematically impossible for things to overflow.