From 8db18055f05898da506e37e3dbbcd2a80f3c7d17 Mon Sep 17 00:00:00 2001 From: Adam Boudjemaa Date: Sun, 29 Mar 2026 11:54:33 +0200 Subject: [PATCH] fix: replace generic Unauthorized() with specific error types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the single generic `Unauthorized()` error with five specific errors to help offchain integrators diagnose issues more precisely: - `UnauthorizedNonSelf()` — caller is not `address(this)` - `UnauthorizedNonOrchestrator()` — caller is not the Orchestrator - `UnauthorizedPayment()` — payment auth failed (wrong caller or account mismatch) - `UnauthorizedPaymasterSignature()` — paymaster signature validation failed - `UnauthorizedSignature()` — intent signature validation failed Closes #399 Co-Authored-By: Claude Opus 4.6 (1M context) --- src/GuardedExecutor.sol | 34 ++++++++++++++++++++++++++-------- src/IthacaAccount.sol | 32 ++++++++++++++++++++------------ test/Account.t.sol | 8 ++++++-- test/GuardedExecutor.t.sol | 28 +++++++++++++++------------- 4 files changed, 67 insertions(+), 35 deletions(-) diff --git a/src/GuardedExecutor.sol b/src/GuardedExecutor.sol index cf5eb502..f87c8a13 100644 --- a/src/GuardedExecutor.sol +++ b/src/GuardedExecutor.sol @@ -86,8 +86,20 @@ abstract contract GuardedExecutor is ERC7821 { /// @dev Only the EOA itself and super admin keys can self execute. error CannotSelfExecute(); - /// @dev Unauthorized to perform the action. - error Unauthorized(); + /// @dev Unauthorized caller, expected `address(this)`. + error UnauthorizedNonSelf(); + + /// @dev Unauthorized caller, expected the Orchestrator. + error UnauthorizedNonOrchestrator(); + + /// @dev Unauthorized payment: caller is not the Orchestrator or account is not the EOA/payer. + error UnauthorizedPayment(); + + /// @dev Unauthorized paymaster signature. + error UnauthorizedPaymasterSignature(); + + /// @dev Unauthorized signature validation failed. + error UnauthorizedSignature(); /// @dev Not authorized to perform the call. error UnauthorizedCall(bytes32 keyHash, address target, bytes data); @@ -384,8 +396,9 @@ abstract contract GuardedExecutor is ERC7821 { if (_isSelfExecute(target, fnSel)) revert CannotSelfExecute(); // Impose a max capacity of 2048 for set enumeration, which should be more than enough. - _getGuardedExecutorKeyStorage(keyHash).canExecute - .update(_packCanExecute(target, fnSel), can, 2048); + _getGuardedExecutorKeyStorage(keyHash).canExecute.update( + _packCanExecute(target, fnSel), can, 2048 + ); emit CanExecuteSet(keyHash, target, fnSel, can); } @@ -408,7 +421,7 @@ abstract contract GuardedExecutor is ERC7821 { // check it in `canExecute` before any custom call checker. EnumerableMapLib.AddressToAddressMap storage checkers = - _getGuardedExecutorKeyStorage(keyHash).callCheckers; + _getGuardedExecutorKeyStorage(keyHash).callCheckers; // Impose a max capacity of 2048 for map enumeration, which should be more than enough. checkers.update(target, checker, checker != address(0), 2048); @@ -517,7 +530,12 @@ abstract contract GuardedExecutor is ERC7821 { /// @dev Returns an array of packed (`target`, `fnSel`) that `keyHash` is authorized to execute on. /// - `target` is in the upper 20 bytes. /// - `fnSel` is in the lower 4 bytes. - function canExecutePackedInfos(bytes32 keyHash) public view virtual returns (bytes32[] memory) { + function canExecutePackedInfos(bytes32 keyHash) + public + view + virtual + returns (bytes32[] memory) + { return _getGuardedExecutorKeyStorage(keyHash).canExecute.values(); } @@ -561,7 +579,7 @@ abstract contract GuardedExecutor is ERC7821 { returns (CallCheckerInfo[] memory results) { EnumerableMapLib.AddressToAddressMap storage checkers = - _getGuardedExecutorKeyStorage(keyHash).callCheckers; + _getGuardedExecutorKeyStorage(keyHash).callCheckers; results = new CallCheckerInfo[](checkers.length()); for (uint256 i; i < results.length; ++i) { (results[i].target, results[i].checker) = checkers.at(i); @@ -680,7 +698,7 @@ abstract contract GuardedExecutor is ERC7821 { /// @dev Guards a function such that it can only be called by `address(this)`. modifier onlyThis() virtual { - if (msg.sender != address(this)) revert Unauthorized(); + if (msg.sender != address(this)) revert UnauthorizedNonSelf(); _; } diff --git a/src/IthacaAccount.sol b/src/IthacaAccount.sol index 0f0068ac..b68ad7b9 100644 --- a/src/IthacaAccount.sol +++ b/src/IthacaAccount.sol @@ -274,8 +274,8 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor { (bool isValid, bytes32 keyHash) = unwrapAndValidateSignature(digest, signature); if (LibBit.and(keyHash != 0, isValid)) { - isValid = _isSuperAdmin(keyHash) - || _getKeyExtraStorage(keyHash).checkers.contains(msg.sender); + isValid = + _isSuperAdmin(keyHash) || _getKeyExtraStorage(keyHash).checkers.contains(msg.sender); } // `bytes4(keccak256("isValidSignature(bytes32,bytes)")) = 0x1626ba7e`. // We use `0xffffffff` for invalid, in convention with the reference implementation. @@ -399,7 +399,12 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor { } /// @dev Returns arrays of all (non-expired) authorized keys and their hashes. - function getKeys() public view virtual returns (Key[] memory keys, bytes32[] memory keyHashes) { + function getKeys() + public + view + virtual + returns (Key[] memory keys, bytes32[] memory keyHashes) + { uint256 totalCount = keyCount(); keys = new Key[](totalCount); @@ -573,8 +578,9 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor { // `keccak256(abi.encode(key.keyType, keccak256(key.publicKey)))`. keyHash = hash(key); AccountStorage storage $ = _getAccountStorage(); - $.keyStorage[keyHash] - .set(abi.encodePacked(key.publicKey, key.expiry, key.keyType, key.isSuperAdmin)); + $.keyStorage[keyHash].set( + abi.encodePacked(key.publicKey, key.expiry, key.keyType, key.isSuperAdmin) + ); $.keyHashes.add(keyHash); } @@ -593,7 +599,7 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor { /// @dev Checks current nonce and increments the sequence for the `seqKey`. function checkAndIncrementNonce(uint256 nonce) public payable virtual { if (msg.sender != ORCHESTRATOR) { - revert Unauthorized(); + revert UnauthorizedNonOrchestrator(); } LibNonce.checkAndIncrement(_getAccountStorage().nonceSeqs, nonce); } @@ -622,11 +628,13 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor { if or(shr(64, t), lt(encodedIntent.length, 0x20)) { revert(0x00, 0x00) } } - if (!LibBit.and( + if ( + !LibBit.and( msg.sender == ORCHESTRATOR, LibBit.or(intent.eoa == address(this), intent.payer == address(this)) - )) { - revert Unauthorized(); + ) + ) { + revert UnauthorizedPayment(); } // If this account is the paymaster, validate the paymaster signature. @@ -650,7 +658,7 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor { } if (!isValid) { - revert Unauthorized(); + revert UnauthorizedPaymasterSignature(); } } @@ -691,7 +699,7 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor { // Simple workflow without `opData`. if (opData.length == uint256(0)) { - if (msg.sender != address(this)) revert Unauthorized(); + if (msg.sender != address(this)) revert UnauthorizedNonSelf(); return _execute(calls, bytes32(0)); } @@ -704,7 +712,7 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor { (bool isValid, bytes32 keyHash) = unwrapAndValidateSignature( computeDigest(calls, nonce), LibBytes.sliceCalldata(opData, 0x20) ); - if (!isValid) revert Unauthorized(); + if (!isValid) revert UnauthorizedSignature(); // TODO: Figure out where else to add these operations, after removing delegate call. LibTStack.TStack(_KEYHASH_STACK_TRANSIENT_SLOT).push(keyHash); diff --git a/test/Account.t.sol b/test/Account.t.sol index d612ef5a..e6e7ef34 100644 --- a/test/Account.t.sol +++ b/test/Account.t.sol @@ -40,7 +40,7 @@ contract AccountTest is BaseTest { signature = _sig(_randomEIP7702DelegatedEOA(), d.d.computeDigest(t.calls, t.nonce)); t.opData = abi.encodePacked(t.nonce, signature); t.executionData = abi.encode(t.calls, t.opData); - vm.expectRevert(bytes4(keccak256("Unauthorized()"))); + vm.expectRevert(bytes4(keccak256("UnauthorizedSignature()"))); d.d.execute(_ERC7821_BATCH_EXECUTION_MODE, t.executionData); return; } @@ -397,6 +397,10 @@ contract AccountTest is BaseTest { vm.prank(address(d.d)); d.d.execute(_ERC7821_BATCH_EXECUTION_MODE, executionData); - assertEq(contextKeyHash, bytes32(0), "Context key hash should be zero for self-execution without opData"); + assertEq( + contextKeyHash, + bytes32(0), + "Context key hash should be zero for self-execution without opData" + ); } } diff --git a/test/GuardedExecutor.t.sol b/test/GuardedExecutor.t.sol index a30c941e..0f1a8ba9 100644 --- a/test/GuardedExecutor.t.sol +++ b/test/GuardedExecutor.t.sol @@ -118,10 +118,9 @@ contract GuardedExecutorTest is BaseTest { // and moved via `transferFrom`. vm.startPrank(d.eoa); - d.d - .setSpendLimit( - k.keyHash, address(paymentToken), GuardedExecutor.SpendPeriod.Day, 1 ether - ); + d.d.setSpendLimit( + k.keyHash, address(paymentToken), GuardedExecutor.SpendPeriod.Day, 1 ether + ); vm.stopPrank(); u.nonce = d.d.getNonce(0); @@ -153,7 +152,7 @@ contract GuardedExecutorTest is BaseTest { DelegatedEOA memory d = _randomEIP7702DelegatedEOA(); PassKey memory k = _randomSecp256r1PassKey(); - vm.expectRevert(bytes4(keccak256("Unauthorized()"))); + vm.expectRevert(bytes4(keccak256("UnauthorizedNonSelf()"))); d.d.setCanExecute(k.keyHash, target, fnSel, true); vm.startPrank(d.eoa); @@ -266,10 +265,9 @@ contract GuardedExecutorTest is BaseTest { assertEq(oc.execute(abi.encode(u)), bytes4(keccak256("NoSpendPermissions()"))); vm.startPrank(d.eoa); - d.d - .setSpendLimit( - k.keyHash, address(paymentToken), GuardedExecutor.SpendPeriod.Day, 1 ether - ); + d.d.setSpendLimit( + k.keyHash, address(paymentToken), GuardedExecutor.SpendPeriod.Day, 1 ether + ); vm.stopPrank(); u.nonce = d.d.getNonce(0); @@ -314,7 +312,7 @@ contract GuardedExecutorTest is BaseTest { ERC7821.execute.selector, _ERC7821_BATCH_EXECUTION_MODE, abi.encode(innerCalls) ); - vm.expectRevert(bytes4(keccak256("Unauthorized()"))); + vm.expectRevert(bytes4(keccak256("UnauthorizedNonSelf()"))); d.d.execute(_ERC7821_BATCH_EXECUTION_MODE, abi.encode(calls)); vm.prank(d.eoa); @@ -550,10 +548,12 @@ contract GuardedExecutorTest is BaseTest { // If first 4bytes are 0xdfc924d5, then it's "anotherTransfer" call, and the spend limit will not catch it. if ( - (calls[0].data[0] == bytes1(uint8(0xdf)) + ( + calls[0].data[0] == bytes1(uint8(0xdf)) && calls[0].data[1] == bytes1(uint8(0xc9)) && calls[0].data[2] == bytes1(uint8(0x24)) - && calls[0].data[3] == bytes1(uint8(0xd5))) || amount == 0 + && calls[0].data[3] == bytes1(uint8(0xd5)) + ) || amount == 0 ) { assertEq(oc.execute(abi.encode(u)), 0); } else { @@ -805,7 +805,9 @@ contract GuardedExecutorTest is BaseTest { _testSpendWithPassKeyViaOrchestrator(_randomSecp256k1PassKey(), address(0)); } - function _testSpendWithPassKeyViaOrchestrator(PassKey memory k, address tokenToSpend) internal { + function _testSpendWithPassKeyViaOrchestrator(PassKey memory k, address tokenToSpend) + internal + { Orchestrator.Intent memory u; GuardedExecutor.SpendInfo memory info;