diff --git a/src/plugins/session/ISessionKeyPlugin.sol b/src/plugins/session/ISessionKeyPlugin.sol index a5270dfd..d2a76467 100644 --- a/src/plugins/session/ISessionKeyPlugin.sol +++ b/src/plugins/session/ISessionKeyPlugin.sol @@ -76,6 +76,7 @@ interface ISessionKeyPlugin { error InvalidToken(address token); error LengthMismatch(); error NativeTokenSpendLimitExceeded(address account, address sessionKey); + error PermissionsCheckFailed(); error SessionKeyNotFound(address sessionKey); // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ diff --git a/src/plugins/session/SessionKeyPlugin.sol b/src/plugins/session/SessionKeyPlugin.sol index 0bdf7721..a5fe0540 100644 --- a/src/plugins/session/SessionKeyPlugin.sol +++ b/src/plugins/session/SessionKeyPlugin.sol @@ -35,7 +35,9 @@ import {Call, IStandardExecutor} from "../../interfaces/IStandardExecutor.sol"; import { AssociatedLinkedListSet, AssociatedLinkedListSetLib } from "../../libraries/AssociatedLinkedListSetLib.sol"; -import {SetValue, SENTINEL_VALUE, SIG_VALIDATION_FAILED} from "../../libraries/Constants.sol"; +import { + SetValue, SENTINEL_VALUE, SIG_VALIDATION_PASSED, SIG_VALIDATION_FAILED +} from "../../libraries/Constants.sol"; import {BasePlugin} from "../BasePlugin.sol"; import {ISessionKeyPlugin} from "./ISessionKeyPlugin.sol"; import {SessionKeyPermissions} from "./permissions/SessionKeyPermissions.sol"; @@ -182,15 +184,20 @@ contract SessionKeyPlugin is ISessionKeyPlugin, SessionKeyPermissions, BasePlugi bytes32 hash = userOpHash.toEthSignedMessageHash(); (address recoveredSig, ECDSA.RecoverError err) = hash.tryRecover(userOp.signature); - if (err == ECDSA.RecoverError.NoError) { - if ( - _sessionKeys.contains(msg.sender, CastLib.toSetValue(sessionKey)) && sessionKey == recoveredSig - ) { - return _checkUserOpPermissions(userOp, calls, sessionKey); - } - return SIG_VALIDATION_FAILED; + if (err != ECDSA.RecoverError.NoError) { + revert InvalidSignature(sessionKey); + } + + if (!_sessionKeys.contains(msg.sender, CastLib.toSetValue(sessionKey))) { + revert PermissionsCheckFailed(); + } + + uint256 validation = _checkUserOpPermissions(userOp, calls, sessionKey); + if (uint160(validation) != uint160(SIG_VALIDATION_PASSED)) { + revert PermissionsCheckFailed(); } - revert InvalidSignature(sessionKey); + // return SIG_VALIDATION_FAILED on sig validation failure only, all other failure modes should revert + return validation | (sessionKey == recoveredSig ? SIG_VALIDATION_PASSED : SIG_VALIDATION_FAILED); } revert NotImplemented(msg.sig, functionId); } diff --git a/src/plugins/session/permissions/SessionKeyPermissions.sol b/src/plugins/session/permissions/SessionKeyPermissions.sol index f8a42ed0..57cd17c8 100644 --- a/src/plugins/session/permissions/SessionKeyPermissions.sol +++ b/src/plugins/session/permissions/SessionKeyPermissions.sol @@ -140,12 +140,10 @@ abstract contract SessionKeyPermissions is ISessionKeyPlugin, SessionKeyPermissi address userOpPaymaster = address(bytes20(userOp.paymasterAndData)); validationSuccess = validationSuccess && (userOpPaymaster == sessionKeyData.requiredPaymaster); } - // Validation return data is 1 in the case of an invalid signature, - // otherwise a packed struct of the aggregator address (0 here), and two - // 6-byte timestamps indicating the start and end times at which the op - // is valid. + // A packed struct of: SIG_VALIDATION_PASSED or SIG_VALIDATION_FAILED, and two + // 6-byte timestamps indicating the start and end times at which the op is valid. return uint160(validationSuccess ? SIG_VALIDATION_PASSED : SIG_VALIDATION_FAILED) - | (uint256(validUntil) << 160) | (uint256(currentValidAfter) << (208)); + | (uint256(validUntil) << 160) | (uint256(currentValidAfter) << 208); } /// @dev Checks permissions on a per-call basis. Should be run during user op validation once per `Call` struct diff --git a/test/plugin/session/SessionKeyPluginWithMultiOwner.t.sol b/test/plugin/session/SessionKeyPluginWithMultiOwner.t.sol index b614c046..c832ee11 100644 --- a/test/plugin/session/SessionKeyPluginWithMultiOwner.t.sol +++ b/test/plugin/session/SessionKeyPluginWithMultiOwner.t.sol @@ -372,7 +372,7 @@ contract SessionKeyPluginWithMultiOwnerTest is Test { assertEq(result, 0); } - function testFuzz_sessionKey_userOpValidation_mismathcedSig(uint8 sessionKeysSeed, uint64 signerSeed) public { + function testFuzz_sessionKey_userOpValidation_mismatchedSig(uint8 sessionKeysSeed, uint64 signerSeed) public { _createSessionKeys(sessionKeysSeed); (address signer, uint256 signerPrivate) = @@ -405,12 +405,11 @@ contract SessionKeyPluginWithMultiOwnerTest is Test { (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivate, userOpHash.toEthSignedMessageHash()); userOp.signature = abi.encodePacked(r, s, v); - vm.prank(address(account1)); - uint256 result = sessionKeyPlugin.userOpValidationFunction( + vm.startPrank(address(account1)); + vm.expectRevert(ISessionKeyPlugin.PermissionsCheckFailed.selector); + sessionKeyPlugin.userOpValidationFunction( uint8(ISessionKeyPlugin.FunctionId.USER_OP_VALIDATION_SESSION_KEY), userOp, userOpHash ); - - assertEq(result, 1); } function testFuzz_sessionKey_userOpValidation_invalidSig(uint8 sessionKeysSeed, uint64 signerSeed) public { diff --git a/test/plugin/session/permissions/SessionKeyERC20SpendLimits.t.sol b/test/plugin/session/permissions/SessionKeyERC20SpendLimits.t.sol index c090aeda..8913667d 100644 --- a/test/plugin/session/permissions/SessionKeyERC20SpendLimits.t.sol +++ b/test/plugin/session/permissions/SessionKeyERC20SpendLimits.t.sol @@ -220,7 +220,7 @@ contract SessionKeyERC20SpendLimitsTest is Test { _runSessionKeyUserOp( calls, sessionKey1Private, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); } diff --git a/test/plugin/session/permissions/SessionKeyGasLimits.t.sol b/test/plugin/session/permissions/SessionKeyGasLimits.t.sol index 1ead5e0a..c1d0d95a 100644 --- a/test/plugin/session/permissions/SessionKeyGasLimits.t.sol +++ b/test/plugin/session/permissions/SessionKeyGasLimits.t.sol @@ -166,7 +166,7 @@ contract SessionKeyGasLimitsTest is Test { 1, 200_000 wei, sessionKey1Private, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); } @@ -206,7 +206,7 @@ contract SessionKeyGasLimitsTest is Test { 1_000 gwei, 0.6 ether, sessionKey1Private, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); } @@ -223,7 +223,7 @@ contract SessionKeyGasLimitsTest is Test { 100_000, 300_000, 1_000 gwei, 0.4 ether, sessionKey1Private, uint256(nonceKey << 64) ); - vm.expectRevert(abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error")); + vm.expectRevert(abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)")); entryPoint.handleOps(userOps, beneficiary); } @@ -240,7 +240,7 @@ contract SessionKeyGasLimitsTest is Test { 2_000 gwei, 1.2 ether, sessionKey1Private, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); } @@ -289,7 +289,7 @@ contract SessionKeyGasLimitsTest is Test { // Run the user ops // The second op (index 1) should be the one that fails signature validation. - vm.expectRevert(abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 1, "AA24 signature error")); + vm.expectRevert(abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 1, "AA23 reverted (or OOG)")); entryPoint.handleOps(userOps, beneficiary); // The lack of usage update should be reflected in the limits @@ -492,7 +492,7 @@ contract SessionKeyGasLimitsTest is Test { // actually usable. skip(1 days + 1 minutes); - vm.expectRevert(abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 2, "AA24 signature error")); + vm.expectRevert(abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 2, "AA23 reverted (or OOG)")); entryPoint.handleOps(userOps, beneficiary); } diff --git a/test/plugin/session/permissions/SessionKeyNativeTokenSpendLimits.t.sol b/test/plugin/session/permissions/SessionKeyNativeTokenSpendLimits.t.sol index 2c16762f..c0579cb9 100644 --- a/test/plugin/session/permissions/SessionKeyNativeTokenSpendLimits.t.sol +++ b/test/plugin/session/permissions/SessionKeyNativeTokenSpendLimits.t.sol @@ -187,7 +187,7 @@ contract SessionKeyNativeTokenSpendLimitsTest is Test { _runSessionKeyUserOp( calls, sessionKey1Private, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); // Run a user op that spends 0 wei, should succeed @@ -233,7 +233,7 @@ contract SessionKeyNativeTokenSpendLimitsTest is Test { _runSessionKeyUserOp( calls, sessionKey1Private, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); } @@ -258,7 +258,7 @@ contract SessionKeyNativeTokenSpendLimitsTest is Test { _runSessionKeyUserOp( calls, sessionKey1Private, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); } @@ -302,7 +302,7 @@ contract SessionKeyNativeTokenSpendLimitsTest is Test { _runSessionKeyUserOp( calls, sessionKey1Private, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); // Assert that the limit is NOT updated @@ -360,7 +360,7 @@ contract SessionKeyNativeTokenSpendLimitsTest is Test { _runSessionKeyUserOp( calls, sessionKey1Private, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); // Assert that the limit is NOT updated @@ -702,7 +702,7 @@ contract SessionKeyNativeTokenSpendLimitsTest is Test { _runSessionKeyUserOp( calls, sessionKey1Private, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); // Assert that limits are NOT updated diff --git a/test/plugin/session/permissions/SessionKeyPermissions.t.sol b/test/plugin/session/permissions/SessionKeyPermissions.t.sol index a6aebfda..4bd801c6 100644 --- a/test/plugin/session/permissions/SessionKeyPermissions.t.sol +++ b/test/plugin/session/permissions/SessionKeyPermissions.t.sol @@ -139,7 +139,7 @@ contract SessionKeyPermissionsTest is Test { sessionKey1Private, abi.encodeCall(Counter.increment, ()), 0 wei, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); // Call should fail before removing the allowlist @@ -198,7 +198,7 @@ contract SessionKeyPermissionsTest is Test { sessionKey1Private, abi.encodeCall(Counter.increment, ()), 0 wei, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); assertEq(counter2.number(), 1); @@ -223,7 +223,7 @@ contract SessionKeyPermissionsTest is Test { sessionKey1Private, abi.encodeCall(Counter.increment, ()), 0 wei, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); assertEq(counter1.number(), 1); @@ -284,7 +284,7 @@ contract SessionKeyPermissionsTest is Test { sessionKey1Private, abi.encodeCall(Counter.setNumber, (5)), 0 wei, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); assertEq(counter1.number(), 2); @@ -314,7 +314,7 @@ contract SessionKeyPermissionsTest is Test { sessionKey1Private, abi.encodeCall(Counter.increment, ()), 0 wei, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); assertEq(counter1.number(), 1); @@ -328,8 +328,12 @@ contract SessionKeyPermissionsTest is Test { } function testFuzz_sessionKeyTimeRange(uint48 startTime, uint48 endTime) public { - bytes[] memory updates = new bytes[](1); + bytes[] memory updates = new bytes[](2); updates[0] = abi.encodeCall(ISessionKeyPermissionsUpdates.updateTimeRange, (startTime, endTime)); + updates[1] = abi.encodeCall( + ISessionKeyPermissionsUpdates.setAccessListType, + (ISessionKeyPlugin.ContractAccessControlType.ALLOW_ALL_ACCESS) + ); vm.prank(owner1); SessionKeyPlugin(address(account1)).updateKeyPermissions(sessionKey1, updates); @@ -395,7 +399,7 @@ contract SessionKeyPermissionsTest is Test { sessionKey1Private, abi.encodeCall(Counter.increment, ()), 0 wei, - abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error") + abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA23 reverted (or OOG)") ); // Attempting to use the new key should succeed @@ -504,15 +508,16 @@ contract SessionKeyPermissionsTest is Test { vm.prank(owner1); SessionKeyPlugin(address(account1)).updateKeyPermissions(sessionKey1, updates2); - vm.prank(address(entryPoint)); - validationData = account1.validateUserOp(userOp, userOpHash, 0); + vm.startPrank(address(entryPoint)); if (requiredPaymaster == providedPaymaster || requiredPaymaster == address(0)) { // Assert that validation passes + validationData = account1.validateUserOp(userOp, userOpHash, 0); assertEq(uint160(validationData), 0); } else { // Assert that validation fails - assertEq(uint160(validationData), 1); + vm.expectRevert(ISessionKeyPlugin.PermissionsCheckFailed.selector); + validationData = account1.validateUserOp(userOp, userOpHash, 0); } }