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

bug: check for address(0) from ecrecover in sig lib. also cleanup lib. #410

Merged
merged 2 commits into from
Aug 1, 2022

Conversation

robrobbins
Copy link
Contributor

No description provided.

@robrobbins robrobbins self-assigned this Aug 1, 2022
@@ -12,7 +12,8 @@ library Sig {

error S();
error V();
error SIG_LENGTH();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no other errors are CONST_STYLE() so remoing in favor of Length. The library same is Sig so i think context lets us know its the signature length

/// @dev splitAndRecover should only be used if it is known that the resulting
/// verifying bit (V) will be 27 || 28. Otherwise use recover, possibly calling split first.
/// @return The recovered address
function splitAndRecover(bytes32 h, bytes memory sig) internal pure returns (address) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this whole method is deleted. it was a bit of legacy that got written and never used. nothing calls this.. the contracts just use recover with split being used by the Fakes and Unit tests.

@robrobbins
Copy link
Contributor Author

image

managed to force ecrecover to return address(0)` and write a spec that shows we now catch it (...failsOnAddress0)


_, err = s.Dep.SigFake.RecoverTest(nil, hash, vrs)

assert.NotNil(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we comment out the new line that reverts on address(0) this spec will fail.

i.e -- it just returns 0x0... and err is nil here


return ecrecover(h, v, r, s);
if (recovered == address(0)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if this check is not present the ...failOn0Address spec fails.

@sourabhmarathe sourabhmarathe self-requested a review August 1, 2022 19:10
Copy link

@sourabhmarathe sourabhmarathe left a comment

Choose a reason for hiding this comment

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

LGTM

@sourabhmarathe sourabhmarathe self-requested a review August 1, 2022 19:10
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.

2 participants