Fix GSNSignatureBouncer signature bug #67
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The GSNSignatureBouncer currently accepts a zero address as a trusted signer. Even more unexpectedly however, is the failure to initialize the GSNSignatureBouncer: In this case the bouncer will accept no signature at all, as _getTrustedSigner() will resolve a zero address for unset values. For people who deploy and fail to initialize their bouncer, their GSN contract will simply "magically" work for anyone who does not provide a signature, but they won't know that because their valid provided signatures will always fail. I've added a catch so that the trustedSigner can not be set to 0x0, and a check so that it will not relay transactions when no `trustedSigner` has been set at all.
|
Also interesting is that the test suite does not catch this error, as it always assumes the contract has been properly initialized. |
crazyrabbitLTC
added a commit
to OpenZeppelin/openzeppelin-contracts
that referenced
this pull request
Sep 17, 2019
Ref: OpenZeppelin/openzeppelin-contracts-upgradeable#67 Current version of the code means the GSNSignatureBouncer will accept Zero Addresses as a trusted signer. The result is that unexpectedly the contract will accept the absence of a signature as a valid signature. The added check prevents the constructor being called with a zero address.
|
I have closed this PR in favor of a more complete PR that includes tests. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The GSNSignatureBouncer currently accepts a zero address as a trusted signer. Even more unexpected however, is that the failure to initialize the GSNSignatureBouncer will allow the bouncer to accept no signature at all.
When uninitialized, _getTrustedSigner() will resolve a zero address for unset values. For people who deploy and fail to initialize their bouncer, their GSN contract will simply "magically" work for anyone who does not provide a signature, but they won't know that even though their valid provided signatures will always fail.
I've added a catch so that the trustedSigner can not be set to 0x0, and a check so that it will not relay transactions when no
trustedSignerhas been set at all.Fixes #
As noted above, the GSNSignatureBouncer allows for Zero addresses for
trustedSignersas well unexpectedly approving GSN transactions when no signature is provided at all.