Adds TimestampedHashRegistry contract #165
Conversation
| ); | ||
| for (uint256 ind = 0; ind < signatures.length; ind++) { | ||
| require( | ||
| signers.contains( |
There was a problem hiding this comment.
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.
btw there's nothing preventing the caller from using the same signature multiple times
There was a problem hiding this comment.
This is kinda expensive. The DapiFallbackV1 version which expected the signatures to be ordered correctly was a good tradeoff imo.
I was really proud with achieving not having to rely on the caller sending the signatures in an expected order 😅
btw there's nothing preventing the caller from using the same signature multiple times
I'm confused by this. Wouldn't the signature check fail if a user sends a signature for a different root or timestamp? Or are you suggesting that I check the root 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.
I mean if I have one valid signature, I can repeat it signers.length times to create a signatures 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 having addSigner()/removeSigner() functions and you should probably avoid using contains() here.
There was a problem hiding this comment.
Got it. Good catch 👏🏻
An alternative that I can think of is creating a new Enumerable.AddressSet for all addresses derived from the signatures. Then loop over the signers set and use contains 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.
There was a problem hiding this comment.
I think using any design other than Safe's is unnecessarily adventurous here. Keep the owners in an AddressSet, have setupSigners(), 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.
Keep the owners in an AddressSet
owners == signers, right?
have
setupSigners(),addSigner(),removeSigner()functions
setupSigners() should completely override the elements in the set, like a reset? or should this be an external function that should only be called once for each hashType to initialize the set of signers (if signers have ever been set for the hashType then revert 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 🤭 )
The CI will ensure that our metadata matches the on-chain order anyway and we can use that to order the signatures accordingly
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.
owners == signers, right?
Right
setupSigners() should completely override the elements in the set, like a reset? or should this be an external function that should only be called once for each hashType to initialize the set of signers (if signers have ever been set for the hashType then revert similar to Safe.sol setup()).
The latter
removeSigner() would change the order of the set since EnumerableSet._remove() swaps the last element of the set with the one being removed 😕
Not a problem. It's the users' (or maybe the manager-multisig frontend's) responsibility to update the metadata accordingly.
(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 🤭 )
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
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?
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.
|
I'm getting a feeling that we won't need this anywhere other than dapi-management so keeping it there will be tidier |
|
I don't care about tests being broken by dfb4ae9 because this contract will be moved to @api3/dapi-management repo where solidity compiler will be set to 0.8.18 |
|
Closing this in favor of this PR |
Closes #102