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
312 stars 77 forks source link

Add check for invalid target. Closes #24 #25

Open gbalabasquer opened 5 years ago

gbalabasquer commented 5 years ago

@rainbreak what do you think about this change? apparently if you delegatecall to an address that doesn't have code it doesn't revert, just passes. I'm not sure if that is ok or not, but as we are checking if the address != 0 we might also check if the target is actually a contract. cc @levity

rainbreak commented 5 years ago

With constantinople you can also consider extcodehash

gbalabasquer commented 5 years ago

With constantinople you can also consider extcodehash

Yeah, I was planning to take care of it in another PR.

gbalabasquer commented 5 years ago

ahh @rainbreak you were meaning for this specific check, I thought you were talking about the possibility of simplifying the cache with the new opcode. Anyway I think using extcodesize is quite clear for this case.

gbalabasquer commented 5 years ago

@rainbreak can we merge this?

rainbreak commented 5 years ago

With constantinople you can also consider extcodehash

Yeah, I was planning to take care of it in another PR.

Are you still considering extcodehash?

gbalabasquer commented 5 years ago

With constantinople you can also consider extcodehash

Yeah, I was planning to take care of it in another PR.

Are you still considering extcodehash?

In the beginning I thought you were suggesting extcodehash for the cache replacement, but then I realized that it was to replace this extcodesize. The only benefit I see using the new opcode is the 300 gas saving. In contraposition the if condition will need check if the result is != 0 and != the empty bytecode hash. I think is clearer for understanding the case using the size opcode. Anyway if you prefer the other, we can wait for Constantinople (if this time succeeds :P).

PaulRBerg commented 3 years ago

As per this other discussion in the OpenZeppelin repo, the gas cost for EXTCODEHASH got set to 700 in EIP-1884. It is not cheaper to use it instead of EXTCODESIZE now.