Make stack.dup explicitly copy the BigNumber

[jochem-brouwer]

Stack.ts dup method states it copies a BigNumber. However, it does not do this; it copies a pointer to BigNumber.

This results into some annoying bugs. For instance, at the CALL operation at opFns.ts the gasLimit is popped from the stack of the VM. If we have to use the "gas stipend" of 2300 gas when we send nonzero Ether, then the iaddn of BN.js is called on this gasLimit number. See the code I'm referring to.

If this gasLimit stack item has been DUPed before, then we are thus changing the number which it originally pointed to, which could still be on the stack!. I found this issue trying to replay mainnet transactions and comparing debug_traceTransaction against VmState traces and seeing that after a call, suddenly an item on the stack has changed which the VM should not even reach at that point.

As a minimal example to show what I mean:

const bn = require('bn.js')

let stackItem = new BN(1)

// clone the stack item in DUP without using clone()

let stackItem_normalCopy = stackItem;
let stackItem_copy = stackItem.clone()
// values should be equal
console.log(stackItem.toString(10), stackItem_normalCopy.toString(10), stackItem_copy.toString(10))
// output: 1 1 1

stackItem_normalCopy.iadd(new BN(1))
stackItem_copy.iadd(new BN(5))

// expect original stack item to be 1, the "normal copy" to be 2, and the copied one to be 6
console.log(stackItem.toString(10), stackItem_normalCopy.toString(10), stackItem_copy.toString(10))
// output: 2 2 6

In this minimal example it seems obvious; but if you start to locally perform arithmetic on BigNumbers it might not be obvious you are operating at a pointer. By cloning the stack item explicitly it is at least ensured that if they are edited elsewhere (which should not happen - if you perform arithmetic on them just for "local" operations, I think they should be cloned!) the DUPd item is not changed.

[jochem-brouwer]

I'll try to convert it to a test, but here is a minimal example:

Test file:
https://pastebin.com/xnMjz53L

Output:
https://pastebin.com/57VtxBa6

[holgerd77]

Hi @jochem-brouwer, thanks for going so deep here, really impressive find, also thanks for providing such explicit notes on the analysis.

Is it possible to create a simple test case in tests/api/evm/stack.ts which fails before your fix from #733 and passes after applying the fix?

If we got a confirmation here this might very well be worth a short-term bugfix v4.1.4 bugfix release. //cc @evertonfraga

[jochem-brouwer]

Hey @holgerd77, I have added a test. It fails before the commit to stack.js and gets accepted after the commit. See the PR. It is a rather large test compared to the others and is maybe not the right place for this test, as it needs access to the VM as well. I'll explain it a bit in the PR but if you want to discuss, I'll be around on gitter.

[holgerd77]

Closed by #733