feat: signature call buffers #240
Conversation
adamegyed
force-pushed
the
adam/call-buffers-sig
branch
from
1a83dd3 to
dc22b1e
Compare
adamegyed
force-pushed
the
adam/deferred-action
branch
from
1aef4f5 to
dea11e5
Compare
adamegyed
force-pushed
the
adam/call-buffers-sig
branch
from
dc22b1e to
5826833
Compare
| @@ -541,9 +540,12 @@ abstract contract ModularAccountBase is | |||
| uoValidation | |||
| ); | |||
|
|
|||
| // Clear the memory after performing signature validation | |||
| MemSnapshot memSnapshot = MemManagementLib.freezeFMP(); | |||
There was a problem hiding this comment.
dude this feels like a sick pattern we could probably use pretty often, but is it safe? Are we sure we're not making obsolete some memory we might not know about?
There was a problem hiding this comment.
I couldn't really think of another place to use it: it doesn't matter for UO validation, and for RT validation, we only allocate the call buffer after loading the pre-validation hooks, and that buffer may be reused for pre exec hooks, so we can't easily free the memory used to hold the pre RT validation hooks.
In theory, the only memory this operation makes unsafe are blocks newly allocated in between the freeze and the restore. Within the internal function _isValidSignature, we allocate an array for the signature validation hooks, and a call buffer for signature validation. Afaict neither of these are reused, nor make it out of the internal function _isValidSignature, so I think we should be fine to deallocate them and reuse that space for the UO validation hooks + UO call buffer..
There was a problem hiding this comment.
Makes sense, my concern is if Solc adds some memory allocations in between. I think it's fine if our tests pass, though.
| mstore(add(buffer, 0x44), entityId) | ||
|
|
||
| // Copy in the signature segment | ||
| // Since the buffer's copy of the signature exceeds the length of any sub-segments, we can safely write |
There was a problem hiding this comment.
Actually @adamegyed is it possible to fuck this up with some sneaky encoding from the user?
There was a problem hiding this comment.
i.e. I encode a length longer than the entire sig for a single segment
There was a problem hiding this comment.
Good thinking! This worried me at first, but I think we're ok.
This should be caught by the behavior of the calldata slices: https://docs.soliditylang.org/en/v0.8.26/types.html#array-slices
If start is greater than end or if end is greater than the length of the array, an exception is thrown.
Looking at our code:
We pass in to SparseCalldataSegmentLib a bounded bytes calldata type, which is either userOp.signature[25:], the version that skips past the deferred action data, bytes calldata authorization, or bytes calldata signature.
In SparseCalldataSegmentLib:
Loading the length:
modular-account/src/libraries/SparseCalldataSegmentLib.sol
Lines 63 to 64 in 6313e6c
Getting the new slice:
So if the reported length exceeds the actual length, the slice creation step should revert.
There was a problem hiding this comment.
Right, good stuff! This means the segment could be incorrect in that it could overreach into other segments (fine, no danger afaik), but it cannot under any circumstance break the invariant of being less than the total signature length. Neato.
There was a problem hiding this comment.
Actually, I think if it "overreaches into the other segments", the other segments would fail to load, because the index and length is only read from after the length of the segment. So the definition of the format makes that kind of overlap impossible.
|
|
||
| if iszero(success) { | ||
| // Bubble up the revert if the call reverts. | ||
| let m := mload(0x40) |
There was a problem hiding this comment.
What's the reasoning behind calling the fmp "m?"
There was a problem hiding this comment.
I think it's just "memory", like "memory we can use". iirc this was a convention that solady was doing for a bit, but I can see how it's not great for readability. What about "mem" or "freeMem"?
There was a problem hiding this comment.
either way works with me. Happy to keep it as is, it's informational.
| if (!_storage.validationData[sigValidation].isSignatureValidation) { | ||
| (address module, uint32 entityId) = sigValidation.unpack(); |
There was a problem hiding this comment.
small idea but can we just emit the module entity without unpacking, what are the implications? (I know we do this in other places too, just wondering if we could save a handful of codesize)
There was a problem hiding this comment.
Probably, we should go back and review our error types to make sure the data included is correct. Like here, I agree we should keep it packed.
There was a problem hiding this comment.
Either way nbd, I don't think it's going to make much difference, but devX is nicer if we don't have to do extra decoding upon receiving an error. Fine either way.
adamegyed
force-pushed
the
adam/call-buffers-sig
branch
from
5826833 to
8c8d350
Compare
adamegyed
force-pushed
the
adam/deferred-action
branch
from
c419355 to
92e2bbb
Compare
adamegyed
force-pushed
the
adam/call-buffers-sig
branch
from
8c8d350 to
e441f28
Compare
Summary by OctaneNew ContractsNo new contracts were added in this PR. Updated Contracts
🔗 Commit Hash: e441f28 |
|
Contract sizes: | Contract | Size (B) | Margin (B) |
|-------------------------------|----------|------------|
| AccountFactory | 4,814 | 19,762 |
| AllowlistModule | 9,230 | 15,346 |
| ECDSAValidationModule | 3,646 | 20,930 |
-| ModularAccount | 25,737 | -1,161 |
+| ModularAccount | 25,574 | -998 |
| NativeTokenLimitModule | 4,714 | 19,862 |
| PaymasterGuardModule | 1,797 | 22,779 |
-| SemiModularAccountBytecode | 27,064 | -2,488 |
-| SemiModularAccountStorageOnly | 27,561 | -2,985 |
+| SemiModularAccountBytecode | 27,562 | -2,986 |
+| SemiModularAccountStorageOnly | 28,080 | -3,504 |
| TimeRangeModule | 2,000 | 22,576 |
| WebauthnValidationModule | 7,854 | 16,722 |Code coverage:
|
This comment was marked as resolved.
This comment was marked as resolved.
Motivation
The last external call that the account makes without call buffers is signature validation, and signature validation hooks.
Solution
Implement call buffers for signature validation.
Add a fuzz test for signature validation inputs.
Reset memory after signature validation in deferred actions to reuse the memory during execution.