Conversation
|
Walkthrough
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (1)test/account/extensions/AccountERC7579.behavior.js (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Not sure if a changeset is required. I would mention the addition of the custom errors but that's probably the only thing to note. People don't rely on errors for critical functionality anyway |
|
I think it could be nice to have a changeset. This is technically breaking |
| ) internal pure virtual returns (address module, bytes calldata innerSignature) { | ||
| return (address(bytes20(signature[0:20])), signature[20:]); | ||
| require(signature.length > 19, ERC7579InvalidModuleSignature()); | ||
| return (address(bytes20(signature)), signature[20:]); |
There was a problem hiding this comment.
I just realized that isValidSignature is already avoiding that this function is called with a signature that's less than 20 bytes long:
function isValidSignature(bytes32 hash, bytes calldata signature) public view virtual returns (bytes4) {
if (signature.length >= 20) {
(address module, bytes calldata innerSignature) = _extractSignatureValidator(signature);
...I would suggest reverting this change and just noting that the function does expect a signature of at least 20 bytes. Otherwise this will add an extra (and arguably unnecessary) check.
There was a problem hiding this comment.
_extractSignatureValidatoris internal (and not private) so it may be called with inputs that are smaller than 20 bytes.- currently, the implementation does a calldata slice before the casting
address(bytes20(signature[0:20])). The creation of that slice is the part checks the length, and cause a panic if the input is not long enough.
The proposal was to skip creation of the slice, and therefore remove the check that can trigger a panic, and do a require instead:
require(signature.length > 19, ERC7579InvalidModuleSignature());
return (address(bytes20(signature)), signature[20:]);This avoid duplicating the check.
In both cases we would do the check only once. Currently the check is implicit (in the slice) and cause a panic. The example above (from 6c8aaca) made that check explicit, with a custom error.
There is a third option with no checks at all, but it doesn't correspond to the current code, and I would argue we should not do it because the function is internal and may be called with invalid inputs.
There was a problem hiding this comment.
Actually, I just realized that the right part signature[20:] also performs a check. Creation of the slice will fail if the signature is not long enough.
So maybe we want to do
return (address(bytes20(signature)), signature[20:]);without the require, and rely on the right part to cause the panic.
There was a problem hiding this comment.
_extractSignatureValidatoris internal ... it may be called with inputs that are smaller than 20 bytes.
Yes! However I thought it could be addressed by documentation exclusively. Right now it's not used on a situation where a signature of less than 20 bytes is passed in, so I thought the right approach was to note it.
Currently the check is implicit (in the slice) and cause a panic. The example above (from 6c8aaca) made that check explicit, with a custom error.
Yeah, I also preferred the implicit check when accessing the slice to keep a panic code and felt the custom error was too opinionated.
return (address(bytes20(signature)), signature[20:]);
This makes sense to me!
ernestognw
left a comment
ernestognw
left a comment
There was a problem hiding this comment.
LGTM if you agree with keeping _extractSignatureValidator as it is right now
Amxx
deleted the
feature/prevent-fallback-module-install-uninstall-without-selector
branch
Co-authored-by: ernestognw <ernestognw@gmail.com> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
3 participants
Followup to #5961
Replaces #5971
PR Checklist
npx changeset add)