From f42d9ff242c72f032d0ea6be36371742576a75eb Mon Sep 17 00:00:00 2001 From: cpb8010 Date: Thu, 19 Dec 2024 02:09:35 -0800 Subject: [PATCH 1/3] feat: add test for session on factory creation (#224) This goes through a slightly different path, it's not specfically intresting, but it's a good example on how to encode the data for session lib session spec --- test/SessionKeyTest.ts | 46 +++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/test/SessionKeyTest.ts b/test/SessionKeyTest.ts index 281d0f50..3e57b704 100644 --- a/test/SessionKeyTest.ts +++ b/test/SessionKeyTest.ts @@ -273,7 +273,11 @@ class SessionTester { await expect(provider.broadcastTransaction(signedTransaction)).to.be.reverted; }; - getLimit(limit?: PartialLimit): SessionLib.UsageLimitStruct { + _getLimit(limit?: PartialLimit): SessionLib.UsageLimitStruct { + return SessionTester.getLimit(limit); + } + + static getLimit(limit?: PartialLimit): SessionLib.UsageLimitStruct { return limit == null ? { limitType: LimitType.Unlimited, @@ -293,28 +297,30 @@ class SessionTester { }; } + + getSession(session: PartialSession): SessionLib.SessionSpecStruct { return { signer: this.sessionOwner.address, expiresAt: session.expiresAt ?? Math.floor(Date.now() / 1000) + 60 * 60 * 24, // unlimited fees are not safe - feeLimit: session.feeLimit ? this.getLimit(session.feeLimit) : this.getLimit({ limit: parseEther("0.1") }), + feeLimit: session.feeLimit ? this._getLimit(session.feeLimit) : this._getLimit({ limit: parseEther("0.1") }), callPolicies: session.callPolicies?.map((policy) => ({ target: policy.target, selector: policy.selector ?? "0x00000000", maxValuePerUse: policy.maxValuePerUse ?? 0, - valueLimit: this.getLimit(policy.valueLimit), + valueLimit: this._getLimit(policy.valueLimit), constraints: policy.constraints?.map((constraint) => ({ condition: constraint.condition ?? 0, index: constraint.index, refValue: constraint.refValue ?? ethers.ZeroHash, - limit: this.getLimit(constraint.limit), + limit: this._getLimit(constraint.limit), })) ?? [], })) ?? [], transferPolicies: session.transferPolicies?.map((policy) => ({ target: policy.target, maxValuePerUse: policy.maxValuePerUse ?? 0, - valueLimit: this.getLimit(policy.valueLimit), + valueLimit: this._getLimit(policy.valueLimit), })) ?? [], }; } @@ -386,21 +392,43 @@ describe("SessionKeyModule tests", function () { it("should deploy proxy account via factory", async () => { const factoryContract = await fixtures.getAaFactory(); const sessionKeyModuleAddress = await fixtures.getSessionKeyModuleAddress(); - const sessionKeyPayload = abiCoder.encode(["address", "bytes"], [sessionKeyModuleAddress, "0x"]); + const transferSessionTarget = Wallet.createRandom().address; + const sessionKeyModuleContract = await fixtures.getSessionKeyContract(); + + // create a session to encode (before the account is deployed) + const args = await factoryContract.getEncodedBeacon(); + const randomSalt = randomBytes(32); + const bytecodeHash = await factoryContract.beaconProxyBytecodeHash(); + const factoryAddress = await factoryContract.getAddress(); + const standardCreate2Address = utils.create2Address(factoryAddress, bytecodeHash, randomSalt, args) ; + let tester = new SessionTester(standardCreate2Address, await fixtures.getSessionKeyModuleAddress()); + const initialSession = await tester.getSession({ + transferPolicies: [{ + target: transferSessionTarget, + maxValuePerUse: parseEther("0.01"), + }], + }); + const initSessionData = abiCoder.encode(sessionKeyModuleContract.interface.getFunction("createSession").inputs, [initialSession]); + const sessionKeyPayload = abiCoder.encode(["address", "bytes"], [sessionKeyModuleAddress, initSessionData]); const deployTx = await factoryContract.deployProxySsoAccount( - randomBytes(32), - "session-key-test-id", + randomSalt, + "session-key-test-id" + randomBytes(32).toString(), [sessionKeyPayload], [fixtures.wallet.address], ); const deployTxReceipt = await deployTx.wait(); + logInfo(`\`deployProxySsoAccount\` gas used: ${deployTxReceipt?.gasUsed.toString()}`); + proxyAccountAddress = deployTxReceipt!.contractAddress!; expect(proxyAccountAddress, "the proxy account location via logs").to.not.equal(ZeroAddress, "be a valid address"); const fundTx = await fixtures.wallet.sendTransaction({ value: parseEther("1"), to: proxyAccountAddress }); - await fundTx.wait(); + const receipt = await fundTx.wait(); + + const initState = await sessionKeyModuleContract.sessionState(proxyAccountAddress, initialSession); + expect(initState.status).to.equal(1, "initial session should be active"); const account = SsoAccount__factory.connect(proxyAccountAddress, provider); assert(await account.k1IsOwner(fixtures.wallet.address)); From 795c857ff01df7afc75ae68518e9a523b7daacf9 Mon Sep 17 00:00:00 2001 From: cpb8010 Date: Thu, 19 Dec 2024 02:11:04 -0800 Subject: [PATCH 2/3] feat: remove uint2str (#225) Unused and doesn't work great according to the audit Co-authored-by: Nicolas Villanueva <1890113+MexicanAce@users.noreply.github.com> --- src/libraries/JsmnSolLib.sol | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/libraries/JsmnSolLib.sol b/src/libraries/JsmnSolLib.sol index 8752e3b0..d0de9229 100644 --- a/src/libraries/JsmnSolLib.sol +++ b/src/libraries/JsmnSolLib.sol @@ -334,23 +334,6 @@ library JsmnSolLib { return mint; } - function uint2str(uint i) internal pure returns (string memory) { - if (i == 0) return "0"; - uint j = i; - uint len; - while (j != 0) { - len++; - j /= 10; - } - bytes memory bstr = new bytes(len); - uint k = len - 1; - while (i != 0) { - bstr[k--] = bytes1(uint8(48 + (i % 10))); - i /= 10; - } - return string(bstr); - } - function parseBool(string memory _a) internal pure returns (bool) { if (strCompare(_a, "true") == 0) { return true; From 79239451a3749d200abfe1919d7a9cbc86b257cf Mon Sep 17 00:00:00 2001 From: cpb8010 Date: Thu, 19 Dec 2024 02:14:49 -0800 Subject: [PATCH 3/3] feat: add values to error messages (#226) also make formatting consistent Co-authored-by: Nicolas Villanueva <1890113+MexicanAce@users.noreply.github.com> --- src/SsoAccount.sol | 2 +- src/auth/BootloaderAuth.sol | 2 +- src/auth/HookAuth.sol | 2 +- src/auth/SelfAuth.sol | 2 +- src/batch/BatchCaller.sol | 4 +-- src/libraries/Errors.sol | 51 +++++++++++-------------------- src/libraries/LinkedList.sol | 40 ++++++++++++------------ src/managers/HookManager.sol | 6 ++-- src/managers/ValidatorManager.sol | 2 +- 9 files changed, 48 insertions(+), 63 deletions(-) diff --git a/src/SsoAccount.sol b/src/SsoAccount.sol index 382e1c1e..20e793c9 100644 --- a/src/SsoAccount.sol +++ b/src/SsoAccount.sol @@ -75,7 +75,7 @@ contract SsoAccount is Initializable, HookManager, ERC1271Handler, TokenCallback // If there is not enough balance for the transaction, the account should reject it // on the validation step to prevent paying fees for revertable transactions. if (_transaction.totalRequiredBalance() > address(this).balance) { - revert Errors.INSUFFICIENT_FUNDS(); + revert Errors.INSUFFICIENT_FUNDS(_transaction.totalRequiredBalance(), address(this).balance); } // While the suggested signed hash is usually provided, it is generally diff --git a/src/auth/BootloaderAuth.sol b/src/auth/BootloaderAuth.sol index 0a55a7b0..9b294f2d 100644 --- a/src/auth/BootloaderAuth.sol +++ b/src/auth/BootloaderAuth.sol @@ -12,7 +12,7 @@ import { Errors } from "../libraries/Errors.sol"; abstract contract BootloaderAuth { modifier onlyBootloader() { if (msg.sender != BOOTLOADER_FORMAL_ADDRESS) { - revert Errors.NOT_FROM_BOOTLOADER(); + revert Errors.NOT_FROM_BOOTLOADER(msg.sender); } _; } diff --git a/src/auth/HookAuth.sol b/src/auth/HookAuth.sol index 540d6c93..79389848 100644 --- a/src/auth/HookAuth.sol +++ b/src/auth/HookAuth.sol @@ -13,7 +13,7 @@ abstract contract HookAuth { modifier onlyHook() { if (!_isHook(msg.sender)) { - revert Errors.NOT_FROM_HOOK(); + revert Errors.NOT_FROM_HOOK(msg.sender); } _; } diff --git a/src/auth/SelfAuth.sol b/src/auth/SelfAuth.sol index d8ad59b2..fc4667fc 100644 --- a/src/auth/SelfAuth.sol +++ b/src/auth/SelfAuth.sol @@ -11,7 +11,7 @@ import { Errors } from "../libraries/Errors.sol"; abstract contract SelfAuth { modifier onlySelf() { if (msg.sender != address(this)) { - revert Errors.NOT_FROM_SELF(); + revert Errors.NOT_FROM_SELF(msg.sender); } _; } diff --git a/src/batch/BatchCaller.sol b/src/batch/BatchCaller.sol index 09b728c5..7561b60e 100644 --- a/src/batch/BatchCaller.sol +++ b/src/batch/BatchCaller.sol @@ -48,12 +48,12 @@ abstract contract BatchCaller is SelfAuth { } if (!_calls[i].allowFailure && !success) { - revert Errors.CALL_FAILED(); + revert Errors.CALL_FAILED(i); } } if (totalValue != msg.value) { - revert Errors.MsgValueMismatch(msg.value, totalValue); + revert Errors.MSG_VALUE_MISMATCH(msg.value, totalValue); } } } diff --git a/src/libraries/Errors.sol b/src/libraries/Errors.sol index 2b2e61d4..8a85aa50 100644 --- a/src/libraries/Errors.sol +++ b/src/libraries/Errors.sol @@ -6,7 +6,7 @@ library Errors { ACCOUNT //////////////////////////////////////////////////////////////*/ - error INSUFFICIENT_FUNDS(); + error INSUFFICIENT_FUNDS(uint256 required, uint256 available); error FEE_PAYMENT_FAILED(); error METHOD_NOT_IMPLEMENTED(); @@ -14,58 +14,43 @@ library Errors { LINKED LIST //////////////////////////////////////////////////////////////*/ - error INVALID_PREV(); + error INVALID_PREV_BYTES(bytes prevValue, bytes oldValue); + error INVALID_PREV_ADDR(address prevValue, address oldValue); // Bytes - error INVALID_BYTES(); - error BYTES_ALREADY_EXISTS(); - error BYTES_NOT_EXISTS(); + error INVALID_BYTES(uint256 length); + error BYTES_ALREADY_EXISTS(bytes length); + error BYTES_NOT_EXISTS(bytes lookup); // Address - error INVALID_ADDRESS(); - error ADDRESS_ALREADY_EXISTS(); - error ADDRESS_NOT_EXISTS(); - - /*////////////////////////////////////////////////////////////// - OWNER MANAGER - //////////////////////////////////////////////////////////////*/ - - error INVALID_PUBKEY_LENGTH(); + error INVALID_ADDRESS(address valid); + error ADDRESS_ALREADY_EXISTS(address exists); + error ADDRESS_NOT_EXISTS(address notExists); /*////////////////////////////////////////////////////////////// VALIDATOR MANAGER //////////////////////////////////////////////////////////////*/ - error VALIDATOR_ERC165_FAIL(); + error VALIDATOR_ERC165_FAIL(address validator); /*////////////////////////////////////////////////////////////// HOOK MANAGER //////////////////////////////////////////////////////////////*/ - error EMPTY_HOOK_ADDRESS(); - error HOOK_ERC165_FAIL(); - error INVALID_KEY(); - - /*////////////////////////////////////////////////////////////// - MODULE MANAGER - //////////////////////////////////////////////////////////////*/ - - error EMPTY_MODULE_ADDRESS(); - error RECURSIVE_MODULE_CALL(); - error MODULE_ERC165_FAIL(); + error EMPTY_HOOK_ADDRESS(uint256 hookAndDataLength); + error HOOK_ERC165_FAIL(address hookAddress, bool isValidation); + error INVALID_KEY(bytes32 key); /*////////////////////////////////////////////////////////////// AUTH //////////////////////////////////////////////////////////////*/ - error NOT_FROM_BOOTLOADER(); - error NOT_FROM_MODULE(); - error NOT_FROM_HOOK(); - error NOT_FROM_SELF(); - error NOT_FROM_SELF_OR_MODULE(); + error NOT_FROM_BOOTLOADER(address notBootloader); + error NOT_FROM_HOOK(address notHook); + error NOT_FROM_SELF(address notSelf); /*////////////////////////////////////////////////////////////// BatchCaller //////////////////////////////////////////////////////////////*/ - error CALL_FAILED(); - error MsgValueMismatch(uint256 actualValue, uint256 expectedValue); + error CALL_FAILED(uint256 batchCallIndex); + error MSG_VALUE_MISMATCH(uint256 actualValue, uint256 expectedValue); } diff --git a/src/libraries/LinkedList.sol b/src/libraries/LinkedList.sol index 6dec0816..f0b02739 100644 --- a/src/libraries/LinkedList.sol +++ b/src/libraries/LinkedList.sol @@ -14,14 +14,14 @@ library BytesLinkedList { modifier validBytes(bytes calldata value) { if (value.length <= SENTINEL_LENGTH) { - revert Errors.INVALID_BYTES(); + revert Errors.INVALID_BYTES(value.length); } _; } function add(mapping(bytes => bytes) storage self, bytes calldata value) internal validBytes(value) { if (self[value].length != 0) { - revert Errors.BYTES_ALREADY_EXISTS(); + revert Errors.BYTES_ALREADY_EXISTS(self[value]); } bytes memory prev = self[SENTINEL_BYTES]; @@ -36,10 +36,10 @@ library BytesLinkedList { function replace(mapping(bytes => bytes) storage self, bytes calldata oldValue, bytes calldata newValue) internal { if (!exists(self, oldValue)) { - revert Errors.BYTES_NOT_EXISTS(); + revert Errors.BYTES_NOT_EXISTS(oldValue); } if (exists(self, newValue)) { - revert Errors.BYTES_ALREADY_EXISTS(); + revert Errors.BYTES_ALREADY_EXISTS(newValue); } bytes memory cursor = SENTINEL_BYTES; @@ -63,13 +63,13 @@ library BytesLinkedList { bytes calldata newValue ) internal { if (!exists(self, oldValue)) { - revert Errors.BYTES_NOT_EXISTS(); + revert Errors.BYTES_NOT_EXISTS(oldValue); } if (exists(self, newValue)) { - revert Errors.BYTES_ALREADY_EXISTS(); + revert Errors.BYTES_ALREADY_EXISTS(newValue); } if (!equals(self[prevValue], oldValue)) { - revert Errors.INVALID_PREV(); + revert Errors.INVALID_PREV_BYTES(self[prevValue], oldValue); } self[newValue] = self[oldValue]; @@ -79,7 +79,7 @@ library BytesLinkedList { function remove(mapping(bytes => bytes) storage self, bytes calldata value) internal { if (!exists(self, value)) { - revert Errors.BYTES_NOT_EXISTS(); + revert Errors.BYTES_NOT_EXISTS(value); } bytes memory cursor = SENTINEL_BYTES; @@ -101,10 +101,10 @@ library BytesLinkedList { bytes calldata value ) internal { if (!exists(self, value)) { - revert Errors.BYTES_NOT_EXISTS(); + revert Errors.BYTES_NOT_EXISTS(value); } if (!equals(self[prevValue], value)) { - revert Errors.INVALID_PREV(); + revert Errors.INVALID_PREV_BYTES(self[prevValue], value); } self[prevValue] = self[value]; @@ -175,14 +175,14 @@ library AddressLinkedList { modifier validAddress(address value) { if (value <= SENTINEL_ADDRESS) { - revert Errors.INVALID_ADDRESS(); + revert Errors.INVALID_ADDRESS(value); } _; } function add(mapping(address => address) storage self, address value) internal validAddress(value) { if (self[value] != address(0)) { - revert Errors.ADDRESS_ALREADY_EXISTS(); + revert Errors.ADDRESS_ALREADY_EXISTS(value); } address prev = self[SENTINEL_ADDRESS]; @@ -197,10 +197,10 @@ library AddressLinkedList { function replace(mapping(address => address) storage self, address oldValue, address newValue) internal { if (!exists(self, oldValue)) { - revert Errors.ADDRESS_NOT_EXISTS(); + revert Errors.ADDRESS_NOT_EXISTS(oldValue); } if (exists(self, newValue)) { - revert Errors.ADDRESS_ALREADY_EXISTS(); + revert Errors.ADDRESS_ALREADY_EXISTS(newValue); } address cursor = SENTINEL_ADDRESS; @@ -224,13 +224,13 @@ library AddressLinkedList { address newValue ) internal { if (!exists(self, oldValue)) { - revert Errors.ADDRESS_NOT_EXISTS(); + revert Errors.ADDRESS_NOT_EXISTS(oldValue); } if (exists(self, newValue)) { - revert Errors.ADDRESS_ALREADY_EXISTS(); + revert Errors.ADDRESS_ALREADY_EXISTS(newValue); } if (self[prevValue] != oldValue) { - revert Errors.INVALID_PREV(); + revert Errors.INVALID_PREV_ADDR(self[prevValue], oldValue); } self[newValue] = self[oldValue]; @@ -240,7 +240,7 @@ library AddressLinkedList { function remove(mapping(address => address) storage self, address value) internal { if (!exists(self, value)) { - revert Errors.ADDRESS_NOT_EXISTS(); + revert Errors.ADDRESS_NOT_EXISTS(value); } address cursor = SENTINEL_ADDRESS; @@ -258,10 +258,10 @@ library AddressLinkedList { function removeUsingPrev(mapping(address => address) storage self, address prevValue, address value) internal { if (!exists(self, value)) { - revert Errors.ADDRESS_NOT_EXISTS(); + revert Errors.ADDRESS_NOT_EXISTS(value); } if (self[prevValue] != value) { - revert Errors.INVALID_PREV(); + revert Errors.INVALID_PREV_ADDR(self[prevValue], value); } self[prevValue] = self[value]; diff --git a/src/managers/HookManager.sol b/src/managers/HookManager.sol index 44db44b0..20a0e6c0 100644 --- a/src/managers/HookManager.sol +++ b/src/managers/HookManager.sol @@ -83,7 +83,7 @@ abstract contract HookManager is IHookManager, Auth { /// @inheritdoc IHookManager function setHookData(bytes32 key, bytes calldata data) external override onlyHook { if (key == CONTEXT_KEY) { - revert Errors.INVALID_KEY(); + revert Errors.INVALID_KEY(key); } _hookDataStore()[msg.sender][key] = data; @@ -161,7 +161,7 @@ abstract contract HookManager is IHookManager, Auth { function _addHook(bytes calldata hookAndData, bool isValidation) internal { if (hookAndData.length < 20) { - revert Errors.EMPTY_HOOK_ADDRESS(); + revert Errors.EMPTY_HOOK_ADDRESS(hookAndData.length); } address hookAddress = address(bytes20(hookAndData[0:20])); @@ -173,7 +173,7 @@ abstract contract HookManager is IHookManager, Auth { function _installHook(address hookAddress, bytes memory initData, bool isValidation) internal { if (!_supportsHook(hookAddress, isValidation)) { - revert Errors.HOOK_ERC165_FAIL(); + revert Errors.HOOK_ERC165_FAIL(hookAddress, isValidation); } if (isValidation) { diff --git a/src/managers/ValidatorManager.sol b/src/managers/ValidatorManager.sol index 99b4fca7..8f02a9c8 100644 --- a/src/managers/ValidatorManager.sol +++ b/src/managers/ValidatorManager.sol @@ -47,7 +47,7 @@ abstract contract ValidatorManager is IValidatorManager, Auth { function _addModuleValidator(address validator, bytes memory accountValidationKey) internal { if (!_supportsModuleValidator(validator)) { - revert Errors.VALIDATOR_ERC165_FAIL(); + revert Errors.VALIDATOR_ERC165_FAIL(validator); } _moduleValidatorsLinkedList().add(validator);