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

Add check for invalid target. Closes #24 #25

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gbalabasquer
Copy link
Contributor

No description provided.

@gbalabasquer
Copy link
Contributor Author

@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

src/proxy.sol Outdated Show resolved Hide resolved
@rainbreak
Copy link
Member

With constantinople you can also consider extcodehash

@gbalabasquer
Copy link
Contributor Author

With constantinople you can also consider extcodehash

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

@gbalabasquer
Copy link
Contributor Author

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
Copy link
Contributor Author

@rainbreak can we merge this?

@@ -58,6 +58,11 @@ contract DSProxy is DSAuth, DSNote {
returns (bytes memory response)
{
require(_target != address(0), "ds-proxy-target-address-required");
uint csize;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use codesize?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't use codesize as variable name in assembly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codeSize or code_size work though. What do you prefer?

@rainbreak
Copy link
Member

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
Copy link
Contributor Author

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
Copy link

PaulRBerg commented Aug 30, 2021

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.

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

Successfully merging this pull request may close these issues.

3 participants