NomicFoundation / hardhat

Hardhat is a development environment to compile, deploy, test, and debug your Ethereum software.
https://hardhat.org
Other
7.29k stars 1.41k forks source link

Delegatecall Fails to Execute Without a Require Success Statement or Return Value #3798

Open m4k2 opened 1 year ago

m4k2 commented 1 year ago

Version of Hardhat

2.13

What happened?

Delegatecall in Hardhat network

This issue pertains to the local network offered by Hardhat and specifically addresses the behavior of a function incorporating a delegatecall. The delegatecall fails to execute when it neither returns a value nor contains a successful require statement.

Minimal reproduction steps

In this scenario, a function from Contract A utilizes a delegatecall to Contract B with the data: 0x95f11c0e (selector for pwned() function). The pwned() function does not return a value, and the delegatecall in Contract A lacks a require statement to ensure success. The primary purpose of this function is to overwrite values in Contract A by executing the pwned() function in Contract B.

This situation is demonstrated in the examples below:

Example 1:

function __delge() public returns (bool){
    (bool success, ) = address(attack).delegatecall(
        abi.encodeWithSelector(0x95f11c0e)
    );

    require(success);
    return success;
}

Example 2:


function __delge() public returns (bool){
    address(attack).delegatecall(
        abi.encodeWithSelector(0x95f11c0e)
    );
}

In the first example, the delegatecall is successful because the require statement ensures that it is successful before returning a boolean value.

In contrast, the second example does not have a require statement to ensure the success of the delegatecall. Therefore, the delegatecall does not execute, and the function returns without any effect.

Search terms

No response

fvictorio commented 1 year ago

Hi @m4k2. If I'm understanding correctly, you are saying that the delegate call in the second example has no effect because the returned value is not checked. This is very unlikely, and I couldn't reproduce it. This is what I did:

contract Delegator {
  uint public x = 1;

  function delegateCall(address bar) public {
    bar.delegatecall(abi.encodeWithSelector(0x95f11c0e));
  }
}

contract Delegated {
  uint x;

  function pwned() public {
    x++;
  }
}

Then I called delegateCall with the address of a deployed Delegated contract, and verified that the state of Delegator changed.

Please create a full, reproducible example if you still think this is an issue in Hardhat. I'm going to tentatively close this, but I'll re-open it if you can provide a full example.

m4k2 commented 1 year ago

This is a full reproductible example :

The .sol file :

pragma solidity ^0.8;

contract B{

    uint public timeStamp;
    function pwned() public{
        timeStamp = block.timestamp;
    }

    fallback() external{}
}

contract A{

    uint public slotOverride;
    B b;

    constructor(){
        b = new B();
    }

    function delegate_1() public{  //simple delegate
        address(b).delegatecall(abi.encodeWithSelector(0x95f11c0e));
    }

    function delegate_2() public{ //delegate with bool success
        (bool success,) = address(b).delegatecall(abi.encodeWithSelector(0x95f11c0e));
    }

    function delegate_3() public{ //delegate with require success
        (bool success,) = address(b).delegatecall(abi.encodeWithSelector(0x95f11c0e));
        require(success);
    }

    function delegate_4() public returns(bool){ //delegate with return success
        (bool success, ) = address(b).delegatecall(abi.encodeWithSelector(0x95f11c0e));

        return success;
    }

    function delegate_5() public returns(bool){ //delegate with require + return : success
        (bool success, ) = address(b).delegatecall(
            abi.encodeWithSelector(0x95f11c0e)
        );

        require(success);
        return success;
    }
}

The Hardhat scripts/deploy.js :

// We require the Hardhat Runtime Environment explicitly here. This is optional
// but useful for running the script in a standalone fashion through `node <script>`.
//
// You can also run a script with `npx hardhat run <script>`. If you do that, Hardhat
// will compile your contracts, add the Hardhat Runtime Environment's members to the
// global scope, and execute the script.
const hre = require("hardhat");

async function main() {

  const A = await hre.ethers.getContractFactory("A");
  const a = await A.deploy();

  await a.deployed();

  // Doesn't work properly
  await a.delegate_1() //simple delegate
  console.log(await a.slotOverride());

  // Doesn't work properly
  await a.delegate_2() //delegate with bool success
  console.log(await a.slotOverride());

  // Work's well
  await a.delegate_3() //delegate with require success
  console.log(await a.slotOverride());

  // Work's well
  await a.delegate_4() //delegate with return success
  console.log(await a.slotOverride());

  // Work's well
  await a.delegate_5() //delegate with (require + return) success
  console.log(await a.slotOverride());
}

// We recommend this pattern to be able to use async/await everywhere
// and properly handle errors.
main().catch((error) => {
  console.error(error);
  process.exitCode = 1;
});

CMDs

npx hardhat node
npx hardhat run --network localhost scripts/deploy.js

Return values

BigNumber { value: "0" }
BigNumber { value: "0" }
BigNumber { value: "1681288070" }
BigNumber { value: "1681288071" }
BigNumber { value: "1681288072" }

Conclusion

It has been noted that the delegate_1() and delegate_2() functions are not operating as expected, as they are unable to overwrite Contract A's first slot during the DelegateCall process.

The delegate call may not execute if there is no 'require' statement or if the success boolean is not returned.

m4k2 commented 1 year ago

@fvictorio, please feel free to ask me if you require any further information :).

t4sk commented 1 year ago

I had a similar problem. In my case, call didn't update state without a require success check.

Using ethers.ContractFactory was causing this problem. https://github.com/t4sk/hardhat-bug/blob/6281aee01b568c47007fff063445ce523abc8f17/test/Call.js#L16-L20

Using await ethers.getContractFactory fixed it. https://github.com/t4sk/hardhat-bug/blob/6281aee01b568c47007fff063445ce523abc8f17/test/Call.js#L22-L27

See here for full code https://github.com/t4sk/hardhat-bug/blob/main/test/Call.js

This might be a bug with ethers.js or hardhat?

fvictorio commented 1 year ago

Hey @m4k2, sorry for not responding before and thanks for the great reproduction steps.

I'm pretty sure this is a bug in Hardhat, not ethers, because I also reproduced it using raw JSON-RPC calls.