Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contract creates a child contract. Child contract is erroneously deleted by the end of the transaction #225

Closed
jwasinger opened this issue Oct 16, 2017 · 5 comments · Fixed by #246

Comments

@jwasinger
Copy link
Contributor

jwasinger commented Oct 16, 2017

This is a bug discovered by @yann300 .

Steps to reproduce:

contract test {
    child c;
    uint p;
    function create () {
        c = new child();
        p = c.get();
    }

    function call () constant returns (uint) {
        if (address(c) != address(0)) {
            return c.get();    
        } else {
            return 0;
        }

    }
}

contract child {
    function get() returns (uint size) { assembly { size := codesize } }
}
@jwasinger
Copy link
Contributor Author

jwasinger commented Oct 16, 2017

Clue: It appears that at the end of the transaction for test.create, child contract c is flagged as empty and deleted. Hence the reason why EXTCODESIZE returns 0 later on (the contract doesn't exist at that point)

@jwasinger jwasinger changed the title EXTCODESIZE bug Contract creates a child contract. Child contract is erroneously deleted by the end of the transaction Oct 20, 2017
@seanavery
Copy link

seanavery commented Oct 21, 2017

Confirming, this bug can be isolated to creating a new contract and calling a method of that contract within the same function scope. If the contract creation and method hit or separated then everything works ok.

As noted above, the bug is occurring somewhere near the end of the create function.

function create() returns (uint) {
        c = new child();
        p = c.get();
         uint length;
        address adrs = address(c);
        assembly {
            length := extcodesize(adrs)
        }
        return length; 
    }

^ This returns the correct code size.

function create() {
        c = new child();
        p = c.get();
    }

 function doesExist() returns (uint) {
        uint length;
        address adrs = address(c);
        assembly {
            length := extcodesize(adrs)
        }
        return length; 
    }

^ doesExist returns empty extcodesize here.

I am noticing some weird remix behavior in the instruction set after the call opcode in c.get()

the ui breaks at this point and no longer displays the instructions nor highlights the execution in the child contract. The stack itself looks fine and is equivalent to when the p = c.get() call is separated into its own function. But in the later case, the remix functionality works fine.

More investigation to come!

@jwasinger
Copy link
Contributor Author

The issue we have here is that contracts created during init code of a another contract are not saved. The fact that this didn't cause any test failures highlights an edge case that is not covered by the state test suite.

@cdetrio
Copy link
Contributor

cdetrio commented Dec 20, 2017

The bug is caused by remix calling _getStorageTrie: https://github.com/ethereum/browser-solidity/blob/f30e2c011d2373749e0ae19bcf6ecba192b27393/src/execution-context.js#L42

That resulted in self.touched.push(address) where address is a string type, then stateManager.touched had a mix of addresses as string types and buffer types. In the loop that checks which touched addresses are empty, the string types always return empty and so get deleted: https://github.com/ethereumjs/ethereumjs-vm/blob/9fd6a1ddda2fd6bb99d9430e0eaf7fc7a6079766/lib/runTx.js#L187-L199

Normally _getStorageTrie is an internal method, so this was only a bug with remix and not a state test edge case.

@yann300
Copy link
Contributor

yann300 commented Dec 20, 2017

wow, nice catch!
Could we add a public function to dump the storage of an address without marking it as touched?
we desperately need it in Remix to have a geth formatted like storage.

btw, this bug might happen in ethereumjs-vm if the classic dumpStorage https://github.com/ethereumjs/ethereumjs-vm/blob/21ee5ed98817e3b6f7840ec9459de10147963c6a/lib/stateManager.js#L274
is used while being in a step, beforeTx or afterTx event.

holgerd77 added a commit that referenced this issue Mar 11, 2021
Update keccak to the latest version 🚀
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment