search

dapphub / ds-proxy

a proxy object that can compose transactions on owner's behalf
https://dapp.tools/dappsys/ds-proxy.html
GNU General Public License v3.0
311 stars 76 forks source link

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

Open levity opened 5 years ago

levity commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

ok, thanks, will try this out.

nmushegian commented 5 years ago

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 commented 5 years ago

@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?