This repository has been archived by the owner on Sep 9, 2024. It is now read-only.
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.
Adds TimestampedHashRegistry contract #165
Adds TimestampedHashRegistry contract #165
Changes from 4 commits
79ba091
c8b2514
b3c82a3
b63d8c6
545a6cc
0a3a22e
9b82003
d4dbc0d
89328b3
9bae6ef
51ea16c
7870e0f
ebd46d6
011bbab
dfb4ae9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kinda expensive. The DapiFallbackV1 version which expected the signatures to be ordered correctly was a good tradeoff imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw there's nothing preventing the caller from using the same signature multiple times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was really proud with achieving not having to rely on the caller sending the signatures in an expected order 😅
I'm confused by this. Wouldn't the signature check fail if a user sends a signature for a different
root
ortimestamp
? Or are you suggesting that I check theroot
parameter is different from a previously registered root for a hash type? Or that the timestamp is no older than a day or something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean if I have one valid signature, I can repeat it
signers.length
times to create asignatures
array that will pass this verification. Note that a signer create multiple valid signatures of the same hash (signature malleability), which means you can't avoid this by preventing signatures from being repeated. All in all, you will have to rely on the caller sending the signatures in the expected order (or sort it for them in the contract), which means the EnumerableSet is only good for havingaddSigner()
/removeSigner()
functions and you should probably avoid usingcontains()
here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Good catch 👏🏻
An alternative that I can think of is creating a new
Enumerable.AddressSet
for all addresses derived from thesignatures
. Then loop over thesigners
set and usecontains
on the derived addresses set to check if a signature for the signer is present. This would allow the caller to send the signatures in any order but it might be more expensive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is making me uneasy tho 🤔 . If we start removing/adding signers then we might assume they are being added at the end of the array while it might be that they are added on an empty slot. My suggestion is to just use array if order matters or use EnumerableSet like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using any design other than Safe's is unnecessarily adventurous here. Keep the owners in an
AddressSet
, havesetupSigners()
,addSigner()
,removeSigner()
functions. Require signatures to be correctly ordered. The CI will ensure that our metadata matches the on-chain order anyway and we can use that to order the signatures accordingly (btw currently the manager-multisig CI doesn't ensure this for Safe or DapiFallbackV1, this needs to be fixed).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
owners == signers, right?
setupSigners()
should completely override the elements in the set, like a reset? or should this be anexternal
function that should only be called once for eachhashType
to initialize the set of signers (if signers have ever been set for thehashType
thenrevert
similar to Safe.sol setup()).removeSigner()
would change the order of the set since EnumerableSet._remove() swaps the last element of the set with the one being removed 😕(btw signatureSplit() only extracts the r,s,v parts from a signature at a specified position but this does not ensure the actual signature order against the previously set owners list, I probably didn't understand the reference tho 🤭 )
Does this mean that after calling
removeSigner()
and the order of the signers changes on-chain we should expect an error in CI and then fix the signers order in our metadata?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right
The latter
Not a problem. It's the users' (or maybe the manager-multisig frontend's) responsibility to update the metadata accordingly.
I mean Safe also depends on sending the signatures in the correct order, sending them in an array or in serialized form doesn't matter
Either that or the frontend should have a "create tx to remove signer" button that you click that updates the metadata as expected and create the transaction at the same time. Once the transaction is executed the CI passes automatically.