064.md

ladboy233

medium

Low level function call does not check the contract existence, transaction that expect to fail can silently go through and the attached ETH is lost

Summary

Low level function call does not check the contract existence, transaction that expect to fail can silently go through

Vulnerability Detail

In the current implementation, the low call function is used when execute a transaction.

library SafeCall {
    /**
     * @notice Perform a low level call without copying any returndata
     *
     * @param _target   Address to call
     * @param _gas      Amount of gas to pass to the call
     * @param _value    Amount of value to pass to the call
     * @param _calldata Calldata to pass to the call
     */
    function call(
        address _target,
        uint256 _gas,
        uint256 _value,
        bytes memory _calldata
    ) internal returns (bool) {
        bool _success;
        assembly {
            _success := call(
                _gas, // gas
                _target, // recipient
                _value, // ether value
                add(_calldata, 0x20), // inloc
                mload(_calldata), // inlen
                0, // outloc
                0 // outlen
            )
        }
        return _success;
    }
}

However, the low level does not check the contract existense if the user means to execute a smart contract transaction.

This low level is performed in OptimismPortal when finalize a withdrawal transaction.

    function finalizeWithdrawalTransaction(Types.WithdrawalTransaction memory _tx) external {

which calls:

// Trigger the call to the target contract. We use SafeCall because we don't
// care about the returndata and we don't want target contracts to be able to force this
// call to run out of gas via a returndata bomb.
bool success = SafeCall.call(
	_tx.target,
	gasleft() - FINALIZE_GAS_BUFFER,
	_tx.value,
	_tx.data
);

The low level is used in CrossDomainMessager.sol when relay the message.

function relayMessage(
	uint256 _nonce,
	address _sender,
	address _target,
	uint256 _value,
	uint256 _minGasLimit,
	bytes calldata _message
) external payable nonReentrant whenNotPaused {

which calls:

xDomainMsgSender = _sender;
bool success = SafeCall.call(_target, gasleft() - RELAY_GAS_BUFFER, _value, _message);
xDomainMsgSender = Constants.DEFAULT_L2_SENDER;

And finally, this low level call is used in StandardBridge.sol

function finalizeBridgeETH(
	address _from,
	address _to,
	uint256 _amount,
	bytes calldata _extraData
) public payable onlyOtherBridge {
	require(msg.value == _amount, "StandardBridge: amount sent does not match amount required");
	require(_to != address(this), "StandardBridge: cannot send to self");
	require(_to != address(MESSENGER), "StandardBridge: cannot send to messenger");

	emit ETHBridgeFinalized(_from, _to, _amount, _extraData);

	bool success = SafeCall.call(_to, gasleft(), _amount, hex"");
	require(success, "StandardBridge: ETH transfer failed");
}

Let us look into the function call again:

* @param _target   Address to call
 * @param _gas      Amount of gas to pass to the call
 * @param _value    Amount of value to pass to the call
 * @param _calldata Calldata to pass to the call
 */
function call(
	address _target,
	uint256 _gas,
	uint256 _value,
	bytes memory _calldata
) internal returns (bool) {

target is the address to call, value is the ETH passed to the call, callData is the callData,

however, if the function call is a smart contract call with call data and ETH attached and the target address to call does not exist, the transaction that expect to fail can sliently go through.

The ETH attached to the call is lost.

https://drive.google.com/file/d/1wEfkUQAY-vmPudJxey0SFgUEcfvbBkw1/view?usp=sharing

As shown in the POC:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.12;
import "forge-std/Test.sol";
import "forge-std/console.sol";

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

import "../src/Executor.sol";

contract ContractNoExist {

    uint256 public balance;

    constructor() {

    }

    function mint() payable public {
        balance += msg.value;
    }

    function over(address user) public payable {
        selfdestruct(payable(user));
    }

}

contract POCTest is Test {
    
    // hacker address
    address hacker = vm.addr(1);
    Executor executor;
    ContractNoExist nocontract;

    function setUp() public {
        executor = new Executor();
        nocontract = new ContractNoExist();
        nocontract.over{value: 1 ether}(hacker);
    }

    function testLackOfCheckInTarget() public {
        bytes memory mintData = abi.encodeWithSelector(ContractNoExist.mint.selector);
        executor.execute{value: 1 ether}(
            address(nocontract), gasleft(), 1 ether, mintData
        );
    }

}

We run

forge test

and the output is:

Running 1 test for test/POC.t.sol:POCTest
[PASS] testLackOfCheckInTarget() (gas: 49418)
Test result: ok. 1 passed; 0 failed; finished in 10.21ms

In the test, we try to call mint function by attaching the mint calldata with ETH and call a that is already self-destructed, which result in lose of ETH.

Impact

if the function call is a smart contract call with call data and ETH attached and the target address to call does not exist, the transaction that expect to fail can sliently go through.

The ETH attached to the call is lost.

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L320-L330

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L322-L326

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/StandardBridge.sol#L283-L310

Tool used

Manual Review, Foundry

Recommendation

If the function call is a smart contract call with call data attached, check the contract existense because executing the function, to not let the transaction that expect to fail sliently execute.