VM: Create collisions using runCode() and runCall()

[alcuadrado]

I'm not completely sure if this should be considered a bug because I don't understand what the intended use case of runCode and runCall methods is, but using them can lead to unexpected CREATE_COLLISION errors.

This can be reproduced like this:

1. Instantiate a VM
2. Add an Account with some ETH.
3. Deploy a contract with that account by sending a TX to the null address.
4. Use runCall to simulate a call to the null address.

runCall will try to deploy the contract, and fail. This is because contract addresses are computed using the caller and their nonce. In this case, the caller is the account that we created, and the nonce is always 0 for runCall and runCode. But we already deployed a contract from the 0-nonce TX of that account, so an address clash happens.

This is a corner case that I found right before shipping Buidler EVM 😅 I was using runCall in our eth_call RPC implementation and started seeing CREATE_COLLISION errors when testing with real codebases. It may seem weird to run a deployment with eth_call, but some projects do it to get revert reasons from failed txs.

My solution was to use use a FakeTransaction and runTx, and revert any state change. I guess that's why FakeTransaction was created in the first place. I really wanted to remove it when migrating -tx to TS, but I'm happy I didn't :)

Should we add nonce to RunCodeOpts and RunCallOpts?

[s1na]

Geez, that's some corner case :)

Should we add nonce to RunCodeOpts and RunCallOpts?

That seems like a fair solution to me.

[atkinsonholly]

@alcuadrado I'll take a look

[alcuadrado]

Feel free to ask any questions. I think the hardest part of this would be deciding where and how to test these changes.

[jochem-brouwer]

We should absolutely add a nonce to RunCodeOpts and RunCallOpts. This is a bit more complex than just passing nonce along the messages; it looks like evm reads this from the State instead of reading it from the message call.

I think a related issue here is #799, we might want to merge these two issues into a single PR?

[jochem-brouwer]

Heya, I thought implementing nonce in RunCodeOpts would make sense, but actually it does not.

The reason is that RunCode calls the interpreter. The interpreter does not care if we are a normal message call or a create message. If it is a create message, this is called from evm.ts in _executeCreate: if the interpreter does not error it will dump the return value of the interpreted code into the contract. It thus makes no sense if we add nonces to runCode.

However, iff runCode would invoke a CREATE opcode, then this will call into EEI and back into the EVM which calls into executeMessage which will then (as per runCall) invoke _executeCreate.

I conclude that it makes no sense to include nonces in RunCodeOpts.

[jochem-brouwer]

Adding this nonce option temporarily changes the nonces of the calling account, runs the call, and then changes it back to the original value. This could lead to undesired (?) side effects where the balance of the account is changed (i.e. besides deducting gas costs the code could also send some ETH over to the sender). I therefore do not know if we want to implement this feature.

@alcuadrado isn't it better to use runTx in this case? Can't you use that instead of using runCall?

Thoughts? @holgerd77 @s1na @ryanio @evertonfraga

[alcuadrado]

@alcuadrado isn't it better to use runTx in this case? Can't you use that instead of using runCall?

Yes, that's actually what I'm doing. I checked ganache's code, and they do the same.

[holgerd77]

What to do with this issue? Close? Or are there still some design adoption considerations to be drawn from here (didn't re-read to closely TBH)?

[ryanio]

just catching up, so @alcuadrado you solved your issue here by moving to runTx?

is anyone actually using runCode or runCall? What are the use cases? I do agree with #799 that it is a confusing clash with web3 terminology. Could we encourage people to use runTx in all cases?

(oops accidentally closed the issue when posting the comment, re-opened)

[alcuadrado]

Yeah, it was solved by moving to runTx.

I don't think runCall has any valid use case tbh. Maybe runCode doesn't have one either? runCall seems to do something that it doesn't, and it's confusing.

[holgerd77]

I think runCode does have its use cases with just running the plain bytecode sequences with not bothering about setting the outer framing with artificially set up a tx or something, see e.g. our usage example in the README, we also use this in various places of the tests.

We might want to consider to deprecate runCall though as being part of the API for v6? Thoughts? 🤔

[alcuadrado]

We might want to consider to deprecate runCall though as being part of the API for v6? Thoughts?

I think that'd make sense.

I agree with the runCode use case.

[gabrocheleau]

@jochem-brouwer Just stumbled into this old issue which seems related to the discussions in #2861

[holgerd77]

Hi @jochem-brouwer, can you please take (short-term) responsibility of this issue? Closing is definitely an option. If you have some alternative suggestion for an implementation please open a new issue with an evolved and concrete proposal and close this issue. Thanks! 🙏

[jochem-brouwer]

Thanks for the ping @holgerd77, took a look, this is the intended behavior.

The EVM is supposed to runCall / runCode on top of some state (which is identified by the state root). Therefore, if you try to first run a call (which creates a contract), and then run the call again (but now on the "new state"), this indeed creates an address clash, but ONLY if the original sender was supposed to be an EOA. In our runTx (which handles EOA calls) there actually is some extra logic which changes the state (in this case, it bumps the nonce of the EOA!). However, if you thus "simulate this EOA call" in the EVM, the sender nonce is not bumped. Therefore, if you run the same transaction, the EVM will see the same nonce and therefore try to deploy it at the same address.

There are two simple ways to get around this:

• The clean one: use runTx from VM to correctly simulate transactions
• The dirty one: bump the nonce after runCall in EVM. This will not correctly simulate EOA calls, because it skips some state-changing logic in runTx, such as charging the tx base cost (21000 in most cases), charging for access lists, etc.

Will close.

[holgerd77]

@jochem-brouwer Thanks for having a look and the detailed "closing-analysis"! 🙏 👍