-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SMA-392: Session Keys V2 #180
Conversation
resolve conflicts |
baf58f7
to
58e7847
Compare
PR is ready to be merged. I think we should merge this post the "all-encompassing audit" |
58e7847
to
1755bb0
Compare
1755bb0
to
c5ca9de
Compare
* needs to be disabled before it expires. | ||
* @param _sessionDigest digest of session key data | ||
*/ | ||
function disableSession(bytes32 _sessionDigest) external; |
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.
Do you think adding a batch disableSession would be helpful. The users can definitely call execute batch multiple times to disableSession but having a method might be useful.
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.
added
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.
Great work! LGTM
contracts/smart-account/modules/SessionKeyManagers/SessionKeyManagerHybrid.sol
Show resolved
Hide resolved
contracts/smart-account/modules/SessionKeyManagers/SessionKeyManagerHybrid.sol
Outdated
Show resolved
Hide resolved
sessionKeyIndex := shr(248, calldataload(offset)) | ||
offset := add(offset, 0x1) | ||
|
||
validUntil := shr(208, calldataload(offset)) | ||
offset := add(offset, 0x6) | ||
|
||
validAfter := shr(208, calldataload(offset)) | ||
offset := add(offset, 0x6) | ||
|
||
sessionValidationModule := shr(96, calldataload(offset)) | ||
offset := add(offset, 0x14) |
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.
sessionKeyIndex := shr(248, calldataload(offset)) | |
offset := add(offset, 0x1) | |
validUntil := shr(208, calldataload(offset)) | |
offset := add(offset, 0x6) | |
validAfter := shr(208, calldataload(offset)) | |
offset := add(offset, 0x6) | |
sessionValidationModule := shr(96, calldataload(offset)) | |
offset := add(offset, 0x14) | |
// Extract the session key index | |
sessionKeyIndex := shr(248, calldataload(offset)) | |
offset := add(offset, 0x1) | |
// Extract the 'valid until' timestamp | |
validUntil := shr(208, calldataload(offset)) | |
offset := add(offset, 0x6) | |
// Extract the 'valid after' timestamp | |
validAfter := shr(208, calldataload(offset)) | |
offset := add(offset, 0x6) | |
// Extract the session validation module address | |
sessionValidationModule := shr(96, calldataload(offset)) | |
offset := add(offset, 0x14) |
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.
In general, I feel that the code is pretty self-explanatory and is self-documenting here.
However, given that I wrote it and am probably biased, I've added the comments in all parse* functions.
contracts/smart-account/modules/SessionKeyManagers/SessionKeyManagerHybrid.sol
Outdated
Show resolved
Hide resolved
// Also find the earliest validUntil and latest validAfter | ||
uint48 earliestValidUntil = type(uint48).max; | ||
uint48 latestValidAfter; | ||
for (uint256 i = 0; i < length; ++i) { |
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.
uncheck ++i
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.
https://soliditylang.org/blog/2023/10/25/solidity-0.8.22-release-announcement/
This is automatically done by the compiler post 0.8.22
contracts/smart-account/modules/SessionKeyManagers/SessionKeyManagerHybrid.sol
Outdated
Show resolved
Hide resolved
...racts/smart-account/interfaces/modules/SessionKeyManagers/ISessionKeyManagerModuleHybrid.sol
Outdated
Show resolved
Hide resolved
...racts/smart-account/interfaces/modules/SessionKeyManagers/ISessionKeyManagerModuleHybrid.sol
Outdated
Show resolved
Hide resolved
...racts/smart-account/interfaces/modules/SessionKeyManagers/ISessionKeyManagerModuleHybrid.sol
Outdated
Show resolved
Hide resolved
...racts/smart-account/interfaces/modules/SessionKeyManagers/ISessionKeyManagerModuleHybrid.sol
Outdated
Show resolved
Hide resolved
/** | ||
* @notice Returns session data for a given session digest and smart account | ||
* @param _sessionDataDigest digest of session key data | ||
* @param _sa smart account for which session key is being disabled |
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.
As per the description.. "smart account for which session key is being disabled"
Is this method specially called for disabling a session for smart account??
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.
The description was incorrect, fixed it
sessionDataDigest_ := calldataload(offset) | ||
offset := add(offset, 0x20) | ||
|
||
let dataPointer := add(baseOffset, calldataload(offset)) |
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.
Didn't understand this line...
isn't calldataload(offset) returns the 32 bytes starting from sessionKeySignature here?
So first 32 bytes of session key signature is some offset value?
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.
Consider the abi encoding of (bytes32 A, bytes B)
.
The length of A
is fixed (32), therefore it is included as it is in the encoded byte array.
However, the length of B
is "dynamic" in that it cannot be known at compile time, and can only be determined at runtime when the actual value of B
is presented for encoding.
Therefore, it is not directly included in the encoding. Instead, the encoded bytes array includes a placeholder pointing to to B
. Once all the parameters have either been encoded in place (fixed size) or their placeholders have been included, the variable length parameters are then appended to the end of the bytes array as <length><actual data>
.
Therefore the actual encoding is
<32 bytes - A> <32 bytes - pointer to B> <32 bytes - len(B)> <len(B) bytes - B>
This placeholder/pointer is simply the offset at which B
and its length are stored(by appending) in the final encoded bytes array. In the code, dataPointer
represents this pointer.
The full specification of this encoding can be found here: https://docs.soliditylang.org/en/develop/abi-spec.html.
|
||
await environment.sendUserOperation(transferUserOp, entryPoint.address); | ||
|
||
expect(await mockToken.balanceOf(charlie.address)).to.equal( |
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.
Should expect the event emitted for SessionCreated
After the transaction, query the contract to see if the session is actually enabled and cross check the count of sessions that are enabled.
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.
The bundler integration tests only check for compatibility with the bundler, and similar positive scenario tests are implemented in foundry that verify all the conditions you've mentioned.
Please refer to
- https://github.com/bcnmy/scw-contracts/blob/feat/sma-392-session-keys-v2/test/foundry/module/SessionKeyManager/SessionKeyManagerHybrid.BatchCall.t.sol#L87
- https://github.com/bcnmy/scw-contracts/blob/feat/sma-392-session-keys-v2/test/foundry/module/SessionKeyManager/SessionKeyManagerHybrid.SingleCall.t.sol#L114
and other positive scenario tests in foundry, where event emissions and state checks have been thoroughly tested. You'll also find tests for enabling and disabling sessions there.
In general, I've implemented all positive and negative tests in foundry, and have only kept basic positive flow tests in bunder/hardhat to ensure bundler compatibility.
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.
The full list of tests implemented in foundry:
Running 18 tests for test/foundry/module/SessionKeyManager/SessionKeyManagerHybrid.SingleCall.t.sol:SessionKeyManagerHybridSingleCallTest
[PASS] testDisableSession() (gas: 208224)
[PASS] testDisableSessions() (gas: 209324)
[PASS] testEnableAndUseSessionInSameTransaction() (gas: 206419)
[PASS] testEnableAndUseSessionMultiSessionEnable() (gas: 267433)
[PASS] testEnableAndUseSessionPostSessionEnable() (gas: 243050)
[PASS] testEnableSession() (gas: 165474)
[PASS] testEnableSessions() (gas: 236559)
[PASS] testExplicitEnableAndUseSessionDifferentOp() (gas: 216171)
[PASS] testShouldNotSupportERC1271SignatureValidation(uint256) (runs: 256, μ: 12261, ~: 12261)
[PASS] testShouldNotSupportERC1271SignatureValidationUnsafe(uint256) (runs: 256, μ: 12195, ~: 12195)
[PASS] testShouldNotValidateTransactionFromNonEnabledSession() (gas: 170546)
[PASS] testShouldNotValidateTransactionFromNonEnabledSessionWithPostCacheFlow() (gas: 119975)
[PASS] testShouldNotValidateTransactionSignedFromInvalidSessionSigner() (gas: 210961)
[PASS] testShouldNotValidateTransactionSignedFromInvalidSessionSignerPostSessionEnable() (gas: 270079)
[PASS] testShouldNotValidateTransactionWithInvalidChainId() (gas: 153673)
[PASS] testShouldNotValidateTransactionWithInvalidSessionIndex() (gas: 153168)
[PASS] testShouldParseEnableSessionSignatureCorrectly(uint8,uint8,uint48,uint48,address,bytes,bytes,bytes,bytes) (runs: 256, μ: 61973, ~: 51556)
[PASS] testShouldParsePreEnabledSignatureCorrectly(uint8,bytes32,bytes) (runs: 256, μ: 21042, ~: 18046)
Test result: ok. 18 passed; 0 failed; 0 skipped; finished in 3.74s
Running 21 tests for test/foundry/module/SessionKeyManager/SessionKeyManagerHybrid.BatchCall.t.sol:SessionKeyManagerHybridBatchCallTest
[PASS] testEnableAndUseSessionSingleBatchItem() (gas: 227729)
[PASS] testEnableAndUseSessionTwoBatchItems() (gas: 332589)
[PASS] testShouldNotAllowInvalidChainIdInSessionEnableData() (gas: 166563)
[PASS] testShouldNotAllowInvalidSessionEnableDataIndex() (gas: 166247)
[PASS] testShouldNotAllowInvalidSessionEnableKeyIndex() (gas: 166860)
[PASS] testShouldNotAllowSessionEnableWithEnableDataListLengthMismatch() (gas: 147112)
[PASS] testShouldNotAllowSessionEnableWithInvalidSessionEnableSignature() (gas: 161367)
[PASS] testShouldNotAllowSessionExecutionIfSVMReturnsDifferentSigner() (gas: 216672)
[PASS] testShouldNotAllowSessionInfosLengthAndBatchItemsLengthMismatch() (gas: 168124)
[PASS] testShouldNotAllowSessionWithDifferentDigestToBeExecuted() (gas: 169453)
[PASS] testShouldNotAllowUsageOfExpiredSession() (gas: 298389)
[PASS] testShouldNotAllowUsageOfNonEnabledSessionsInPreEnabledFlow() (gas: 132143)
[PASS] testShouldNotAllowUsageOfSessionsNotValidYet() (gas: 298728)
[PASS] testShouldNotSupportERC1271SignatureValidation(uint256) (runs: 256, μ: 12287, ~: 12287)
[PASS] testShouldNotSupportERC1271SignatureValidationUnsafe(uint256) (runs: 256, μ: 12111, ~: 12111)
[PASS] testShouldParseBatchCallDataCorrectly(address[],uint256[],bytes[]) (runs: 256, μ: 4337881, ~: 4337168)
[PASS] testShouldParseSessionDataPreEnableSignatureBatchCall(uint8,bytes32,bytes) (runs: 256, μ: 20731, ~: 17927)
[PASS] testShouldParseSessionEnableSignatureCorrectly(uint8,uint8,uint8,uint48,uint48,address,bytes,bytes) (runs: 256, μ: 35498, ~: 29535)
[PASS] testShouldParseValidateUserBatchSignature(bytes[],bytes[],bytes[],bytes) (runs: 256, μ: 13238584, ~: 13069998)
[PASS] testUseSessionTwoBatchItemsOneFreshAndOtherPostEnable() (gas: 389251)
[PASS] testUseSessionTwoBatchItemsPostEnable() (gas: 387304)
Test result: ok. 21 passed; 0 failed; 0 skipped; finished in 10.71s
); | ||
}); | ||
|
||
it("Should process signed user operation from Session when session is pre-enabled", async () => { |
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.
- Add negative test cases as well, that will test expired sessions should not allow executing any userOp signed by session key.
- Invalid session key data format, should be reverted.
- Add test cases for disableSession and disableSessions
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.
All negative tests are implemented in foundry, please refer to tests starting from
- https://github.com/bcnmy/scw-contracts/blob/feat/sma-392-session-keys-v2/test/foundry/module/SessionKeyManager/SessionKeyManagerHybrid.BatchCall.t.sol#L526
- https://github.com/bcnmy/scw-contracts/blob/feat/sma-392-session-keys-v2/test/foundry/module/SessionKeyManager/SessionKeyManagerHybrid.SingleCall.t.sol#L563
Looks great 🚀 LGTM |
Should we merge this or Not? |
@Aboudjem please let us know when this can be safely merged. |
Sure ! let's discuss it next week |
@Aboudjem Any update on this? |
I thought we had agreed to proceed with the merging after completing the audits, if I'm not wrong |
This PR has been inactive for 30 days. If it's waiting for a review, please reach out to the team. Otherwise, please update the PR or it will be closed due to inactivity. |
We have completed 1 audit for the module, I believe it can be safely merged now. |
82abd25
to
c3fe846
Compare
Introduction
Introduces a new
SessionKeyManagerHybrid
Module to support Session Keys infrastructure with the following enhancements:SessionData(svm, validUntil, validAfter, data)
on-chain. This removes any dependency on any off-chain DA solution such as Merkle tree storage, removing the need for synchronization between DApps using Session Keys infrastructure.bytes32 sessionDigest
for subsequent session transactions, resulting in massive calldata savings for long-running sessions. This results in significantly cheaper transactions on L2s (amortized costs).execute
andexecuteBatch
flows, eliminating the need to separately integrate the Batched Session Router to support sessions while batching.Other notes:
execute
andexecuteBatch
modes.abi.encodePacked
to reduce calldata size to further the gas savings on L2s.Usage
Definitions
Enabling a Session Manually
Disabling a Session Manually
Normally a session will expire automatically based on
(validUntil, validAfter)
, however an option to disable the session manually is available.Single Execute
Enable And Use Session
Prepare a User Operation where
op.calldata[0:4]
isSmartAccount.execute.selector
orSmartAccount.execute_ncC.selector
.Prepare one or more
SessionData
as described above. CalculatePrepare
bytes sessionEnableData
asSign
sessionEnableData
usingEIP1271
to producesessionEnableSignature
Prepare
moduleSignature
asUse Pre-Enabled Session
Prepare a User Operation where
op.calldata[0:4]
isSmartAccount.execute.selector
orSmartAccount.execute_ncC.selector
.No
sessionEnableData
is needed in this case. Simply preparemoduleSignature
asBatch Execute
Prepare a User Operation where
op.calldata[0:4]
isSmartAccount.executeBatch.selector
orSmartAccount.executeBatch_y6U.selector
.Here, for each item of the batch independently follow either the enable-and-use or the pre-enabled flow.
The global
moduleSignature
structure is as follows:First some invariants:
sessionInfo.length == executeBatch-operations.length
sessionEnableDataList.length == sessionEnableSignatureList.length >= 0
Each item in
sessionInfo
corresponds to an operation in the batch.If any session wants to leverage the enable-and-use flow, it can refer to one of the enableData-signature pair. If no sessions use these flows, the first two lists are empty.
Session Enable and Signature List
Follows the same structure as described in the Single Execute Section. This is effectively a 2D array of
(sessionDataHash, chainID)
. Assuming that batch operations A, B and C reference sessions SA, SB and SC, assuming the following Session Enable Data:sessionInfo[0]
can refer to it's corresponding SED entry as[0, 0]
sessionInfo[1]
can refer to it's corresponding SED entry as[0, 1]
sessionInfo[2]
can refer to it's corresponding SED entry as[1, 0]
Session Info Structure - Enable And Use
Session Info Structure - Use Pre-Enabled Session
Change Type
Checklist