Skip to content

fix: replace generic Unauthorized() with specific error types #427

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.

Sign up for GitHub

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

Open
Aboudjem wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: replace generic Unauthorized() with specific error types #427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions src/GuardedExecutor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

Expand All @@ -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);
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
_;
}

Expand Down
32 changes: 20 additions & 12 deletions src/IthacaAccount.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

Expand All @@ -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);
}
Expand Down Expand Up @@ -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.
Expand All @@ -650,7 +658,7 @@ contract IthacaAccount is IIthacaAccount, EIP712, GuardedExecutor {
}

if (!isValid) {
revert Unauthorized();
revert UnauthorizedPaymasterSignature();
}
}

Expand Down Expand Up @@ -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));
}

Expand All @@ -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);
Expand Down
8 changes: 6 additions & 2 deletions test/Account.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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"
);
}
}
28 changes: 15 additions & 13 deletions test/GuardedExecutor.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;

Expand Down