crytic / etheno

Simplify Ethereum security analysis and testing
GNU Affero General Public License v3.0
332 stars 31 forks source link

Etheno depends on transaction receipt requests for exporting with "-x" #93

Open rappie opened 2 years ago

rappie commented 2 years ago

Hi.

I've been having problems with exporting my deployment transactions using Hardhat. It turns out this is because the etheno logging system depends on a eth_getTransactionReceipt for it's exporting plugin (EventSummaryExportPlugin). Hardhat does not request this receipt by default for function calls, so only contract deployments are being logged. Truffle does request this by default for both functions and deployments. I have not tested it in any other frameworks.

When I specifically request a receipt in the hardhat deploy script I can get it to work using the following code:

    const tx = await simple.doSomething();  // function call
    const receipt = await tx.wait(); // request receipt

Right now this is something that has to be remembered every time to keep an accurate etheno export. People new to etheno will probably not know this at all. I think it would be an nice improvement if etheno can support this out of the box. Without the need for manually requesting the receipt.

I have been tinkering with the etheno code a bit, looking for a fix. I've come up with 2 ideas for a possible fixes. Each have their own problems though, i'm hoping you guys can shed some light on this.

  1. Log function call transactions directly in eth_sendRawTransaction because you dont need to know the address of a deployed contract (which you can only know afterwards). The problem here is that currently "gas used" is also being logged. This is something you can't know in advance either, right? Is it important to keep logging this?
  2. Dont depend on the user to call eth_getTransactionReceipt at all. Etheno could just call this itself after every transaction. This would be the most robust fix in my opinion. The problem here is that we cannot do this right away, because the transaction is still being mined. So using the after_post hook for this if impossible. Correct me if i'm wrong. We'd have to have another kind of hook, or can we use evm_mine for this?

I'd be interested in putting a bit of time into this. So far i've been looking at this for about 2-4 hours. I expect one of these fixes to take another 4-8 hours (if they turn out to be possible).

Let me know what you think and if this would be elligible for a bounty. Thanks.

ggrieco-tob commented 2 years ago

I think we need to discuss #95 first, to avoid modifying some code that we will need to rewrite later :disappointed:

rappie commented 2 years ago

I've made some hacky changes to be able to continue my current work on the Damn Vulnerable DeFi challenges:

https://github.com/rappie/etheno/commit/574eff0e20291b57c24390efc6e2024cb78bf97c

anishnaik commented 2 years ago

Hi @rappie thanks for bringing this up, this will be a great feature addition. After thinking through it for a bit, the simplest option seems to be to use the before_post hook to identify which transactions don't have a receipt. We can identify which transactions don't have receipts through the self._transactions mapping in jsonrpc.py (with some minor modifications) and then send a post request to get the receipt for it. Then this receipt can be logged.

So the flow would be:

  1. Block 1: MyContract.callFunc()
  2. Block 2: use the beforeHook to get the receipt for that call

The main edge case that comes to mind is that any function call(s) made in the last block won't be checked for receipts since no additional before_post hooks will be called.

We can try to squeeze in this feature in an upcoming release. I'll come up with a final design and post here for your thoughts.

anishnaik commented 2 years ago

@rappie so I've been working on a bug fix for this here: https://github.com/crytic/etheno/commit/6fb5725f50ce043f94cdbda11a91ab902d10bd4e#diff-5cd6b2cb236946ec22c8de58120240c893c21e015c0ab414a8b2f2feccdb3e0b

The general idea here was to not call eth_getTransactionReceipt unless necessary. So, what I did was track all the transactions that were not logged and upon shutdown of the system, make etheno call eth_getTransactionReceipt on all outstanding transactions. You can see the main changes in the jsonrpc.py file in the commit above.

The one edge case I noticed is if the last transaction to be made is a contract creation call.

describe("Deploy SimpleStorage", function() {
  it("Deploy", async function() {
    // Get Factory
    const SimpleStorageFactory = await ethers.getContractFactory("SimpleStorage");
    // Deploy
    const SimpleStorage = await SimpleStorageFactory.deploy();

    const [account] = await ethers.getSigners()
    await SimpleStorage.set(89, { from: account.address });
    // Check stored value
    expect(await SimpleStorage.storedData()).to.equal(89);
    // Deploy another
    const SimpleStorageTwo = await SimpleStorageFactory.deploy();

  });
});

If you run this branch on the code above, etheno will hang and never successfully complete a call using eth_getTransactionReceipt. My guess is because Ganache does not actually deploy the contract unless there is some action that is performed on that contract (SimpleStorageTwo). For example, if I change the code above to:

describe("Deploy SimpleStorage", function() {
  it("Deploy", async function() {
    // Get Factory
    const SimpleStorageFactory = await ethers.getContractFactory("SimpleStorage");
    // Deploy
    const SimpleStorage = await SimpleStorageFactory.deploy();

    const [account] = await ethers.getSigners()
    await SimpleStorage.set(89, { from: account.address });
    // Check stored value
    expect(await SimpleStorage.storedData()).to.equal(89);
    // Deploy another
    const SimpleStorageTwo = await SimpleStorageFactory.deploy();

    // Set again
    await SimpleStorageTwo.set(42, { from: account.address });

  });
});
  });
});

then since SimpleStorageTwo is used, the eth_getTransactionReceipt call will succeed on SimpleStorageTwo. I'm going to open an issue in the ganache repo to see if this is expected or a bug. Let me know what you think of this so far.

anishnaik commented 2 years ago

This commit should fix the issue: https://github.com/crytic/etheno/commit/676ef39935f9f5367533d7e08251f5201958f70e

The problem was that we converted transaction hashes into integers and then back to a hex string. This can lead to the truncation of leading zeros.

Give it a try if you have a chance @rappie

rappie commented 2 years ago

Awesome, I will take a look soon.

You can add the following line to fix problems with contract deployments not being logged:

await SimpleStorageTwo.deployed()

I have discovered this recently and haven't investigated why it works. I assume it forces hardhat to do an eth_getTransactionReceipt request.

anishnaik commented 2 years ago

Yup, .deployed() works as well but the issue with that is we have to clearly specify that in the documentation and is a solvable edge case. Depending on what unit tests are run with etheno, the .deployed() might have to be added in a variety of places which decreases the usability of the tool. The commit I mentioned here (https://github.com/crytic/etheno/commit/676ef39935f9f5367533d7e08251f5201958f70e) solves both the function call issue and contract deployment isssue.

Btw, the .deployed(), like you mentioned, enforces the eth_getTransactionReceipt call.

rappie commented 2 years ago

I have tested the your changes and everything seems to be working.

There are some differences in the order of transactions being logged compared to my own (hacky) bugfixed version. I dont know which one is correct and it doesnt seem to be causing any problems.

I've switched to the RC1 branch as my daily driver. I'll let you know if I run into any problems.

Thanks :smile:

anishnaik commented 2 years ago

That's great to hear @rappie - thank you for your help in testing this out. The order will be different but I don't believe that that should cause any side effects. However, to make sure I will chat with Gustavo to see what happens from the Echidna side.