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

Calling execute with an invalid address doesn't revert? #24

Open
levity opened this issue Jan 18, 2019 · 5 comments
Open

Calling execute with an invalid address doesn't revert? #24

levity opened this issue Jan 18, 2019 · 5 comments
Assignees

Comments

@levity
Copy link

levity commented Jan 18, 2019

i.e. if i call it with address(0) it reverts with "ds-proxy-target-address-required" as expected, but if i call it with 0x0000000000000000000000000000000000000001 no revert occurs. Is this expected delegatecall behavior?

@gbalabasquer
Copy link
Contributor

gbalabasquer commented Jan 18, 2019

the require happens before the delegatecall https://github.com/dapphub/ds-proxy/blob/master/src/proxy.sol#L60
But if the call fails, should also revert. Which is the exact example you are trying and what is the result?

@levity
Copy link
Author

levity commented Jan 18, 2019

I noticed this when working in the mcd branch of Dai.js on the ganache testchain. I created a snapshot with a deployment of MomLib and configured Dai.js to use it to change the debt ceiling. That worked fine. But when I changed the snapshot back to one that didn't have MomLib deployed, and ran the same code, the MomLib call still completed without an error.

If I change the config so MomLib's address is the address of some other contract in the snapshot, like the ETH price feed, then it does revert. But if the address is not 0, but it's an address with no contract attached, then it doesn't revert.

@gbalabasquer
Copy link
Contributor

ok, thanks, will try this out.

@gbalabasquer gbalabasquer self-assigned this Jan 21, 2019
@nmushegian
Copy link
Member

What defines an invalid target? Not having code? But you still have to be able to transfer to a key... Unfortunately I think this has to remain as the ugly behavior and technically even removing the 0x0 special case might be the correct thing.

I should catch up on EVM updates before making too many comments

@gbalabasquer
Copy link
Contributor

@nmushegian if you want to transfer ETH to a key from your proxy, you would need to delegatecall to a contract that does that.
I can imagine something like:

contract Send {
	function transfer(address guy, uint wad) public {
		require(guy.send(wad), "Error transferring ETH");
	}
}

If you basically delegatecall to no code, you would be doing nothing at all. Or am I missing anything?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants