Simplification & Fixes in Orchestrator and Account #258

legion2002 · 2025-07-22T14:35:02Z

Solves issues mentioned in the audit around misuse of combinedGas field, and minimum gas needed by the execute function.
Removes all simulation logic from deployed contracts, making core contracts even simpler, without any loss of accuracy. Separate Simulation Contracts from Deployed Contracts #152
Extracts batching of intents from the core contracts to the periphery. Which allows us to remove a lot of ugly logic around handling errors. The current way gives a lot of flexibility to the relay to batch intents how they want. The resulting errors are also more detailed, and should lead to easier debugging.

Note all of the changes, come at no regression in gas costs or features, but lead to reducing a lot of tech debt, and solve a bunch of audit issues.

github-actions · 2025-07-22T14:35:15Z

Claude finished @legion2002's task —— View job

✅ PR Review Complete

I've completed a comprehensive review of PR #258: "Simplification & Fixes in Orchestrator and Account"

Review Summary:

Files Reviewed: 9 changed files across +406/-513 lines
Comments Added: 8 inline review comments with specific suggestions
Overall Assessment: Excellent refactor that addresses audit issues while dramatically simplifying the codebase

📋 Review Checklist - All Complete

Todo List:

Review Batcher.sol security and design
Review Orchestrator.sol gas model changes (combinedGas → executeGas)
Review simulation logic removal from production contracts
Review new simulation contracts (IthacaAccountSim.sol, OrchestratorSim.sol)
Review security implications of simplified execute flow
Review assembly code changes and memory safety
Check compliance with project guidelines (CEI, error handling, etc.)
Submit comprehensive GitHub review

🎯 Key Findings

✅ Strengths:

Addresses audit concerns around combinedGas misuse
Removes 237 lines from Orchestrator.sol through smart simplification
Clean separation of simulation vs production logic
Enhanced security with better gas validation
Maintains all functionality while reducing complexity

⚠️ Areas for Improvement:

Batcher contract needs gas limit validation (see inline comments)
TODO comments should be resolved or clarified
Minor documentation improvements suggested

🚀 Recommendation: Approve with minor fixes to Batcher security concerns.

This is an exemplary refactor that successfully achieves the rare combination of fixing audit issues, dramatically reducing complexity, and maintaining security posture.

github-actions

Comprehensive Review: Orchestrator Simplification & Fixes

This PR represents a significant architectural improvement that successfully addresses audit concerns while dramatically simplifying the codebase. The changes align excellently with the project's "prefer simplicity" and security-first principles.

🎯 Key Achievements

✅ Audit Issues Resolved:

Fixed combinedGas field misuse by renaming to executeGas with precise semantics
Improved minimum gas handling with proper validation checks
Eliminated complex simulation flags from production contracts

✅ Major Simplification:

Removed 237 lines from Orchestrator.sol
Eliminated complex error code returns and simulation modes
Cleaner, more auditable production contracts

✅ Architectural Improvements:

Clean separation of simulation vs production logic
New dedicated simulation contracts that inherit from production ones
Simplified event emissions and gas flow

🔒 Security Analysis

Strengths:

Enhanced gas validation with proper 63/64 rule application
Removed potential attack surfaces from embedded simulation logic
Better access control and fail-fast validation
Maintains CEI pattern and proper error handling

Areas of Attention:

Batcher contract needs gas limit validation to prevent griefing
TODO comments should be resolved or clarified
Consider documenting breaking changes in simulation API

📊 Code Quality Assessment

Follows Project Guidelines: ✅ Custom errors, memory-safe assembly, proper documentation
Maintains Security: ✅ Reentrancy protection, input validation, gas safety
Improves Maintainability: ✅ Simpler control flow, clearer separation of concerns

🚀 Recommendations

Address Batcher security (see inline comments)
Resolve/clarify TODO comments about gas override and encoding
Document breaking changes in simulation contracts
Run comprehensive gas benchmarks to verify no regressions

💡 Overall Assessment

This is an exemplary refactor that achieves the rare combination of:

✅ Fixing audit issues
✅ Dramatically reducing complexity
✅ Maintaining security posture
✅ Preserving functionality

The separation of simulation logic into dedicated contracts is particularly well-designed, maintaining accuracy while cleaning up the production codebase.

Recommendation: Approve with minor fixes to the Batcher contract and TODO resolution.

src/Batcher.sol

github-actions · 2025-07-22T14:37:22Z

src/Orchestrator.sol

    /// @dev Executes a single encoded intent.
    /// `encodedIntent` is given by `abi.encode(intent)`, where `intent` is a struct of type `Intent`.
    /// If sufficient gas is provided, returns an error selector that is non-zero
    /// if there is an error during the payment, verification, and call execution.


Simplified Execute Function - Good Change

✅ Excellent simplification! Removing the complex error return codes and simulation flags makes the production contract much cleaner and easier to audit. This aligns perfectly with the project's "prefer simplicity" guideline.

The function now has a single clear responsibility: execute an intent and revert on failure. This is much safer than the previous approach of returning error codes that callers might not handle properly.

github-actions · 2025-07-22T14:37:49Z

src/Orchestrator.sol

    /// @dev For EIP712 signature digest calculation for the `execute` function.
    bytes32 public constant INTENT_TYPEHASH = keccak256(
-        "Intent(bool multichain,address eoa,Call[] calls,uint256 nonce,address payer,address paymentToken,uint256 prePaymentMaxAmount,uint256 totalPaymentMaxAmount,uint256 combinedGas,bytes[] encodedPreCalls,bytes[] encodedFundTransfers,address settler,uint256 expiry)Call(address to,uint256 value,bytes data)"
+        "Intent(bool multichain,address eoa,Call[] calls,uint256 nonce,address payer,address paymentToken,uint256 prePaymentMaxAmount,uint256 totalPaymentMaxAmount,uint256 executeGas,bytes[] encodedPreCalls,bytes[] encodedFundTransfers,address settler,uint256 expiry)Call(address to,uint256 value,bytes data)"


Gas Model Improvement: combinedGas → executeGas

✅ Great change! The rename from combinedGas to executeGas is much more semantically accurate and addresses the audit concern about misuse of the gas field.

Key improvements:

Clearer semantics: executeGas clearly indicates this gas is specifically for the account execution call

Audit compliance: Addresses misuse of the previous combinedGas field mentioned in audit

More precise gas control: The gas is now used exactly where intended (line 340 in selfCallExecutePay)

The EIP712 type hash correctly reflects this change, maintaining signature compatibility for the new model.

github-actions · 2025-07-22T14:37:59Z

src/Orchestrator.sol

+            // This ensures that a relayer cannot grief the user's execution, by making the execute fail
+            // because of the maximum allowed call depth in the EVM.
+            // This also ensures that there is enough gas left after the execute to complete the
+            // remaining flow after the self call.


Critical Security: Gas Check Implementation

✅ Excellent addition of the gas sufficiency check! This addresses a major security concern.

Security benefits:

Prevents execution at high call depth: Protects against griefing attacks that could cause execution failure

Ensures remaining gas for post-execution flow: Critical for completing payment and cleanup operations

Uses 63/64 rule correctly: Proper application of EVM gas forwarding rules

Suggestion for clarity:

Suggested change

// remaining flow after the self call.

// Ensure sufficient gas for safe execution:

// - Prevents execution at dangerous call depth

// - Reserves gas for post-execution operations (payments, cleanup)

// - Uses 63/64 gas forwarding rule with safety buffer

if (((gasleft() * 63) >> 6) < Math.saturatingAdd(i.executeGas, _INNER_GAS_OVERHEAD)) {

This is a much better approach than the previous complex gas validation that was mixed with simulation logic.

github-actions · 2025-07-22T14:38:38Z

src/simulation/IthacaAccountSim.sol

+        returns (bool isValid, bytes32 keyHash)
+    {
+        (isValid, keyHash) = super.unwrapAndValidateSignature(digest, signature);
+


Clean Simulation Architecture

✅ Excellent approach to simulation separation!

Benefits:

Inheritance-based: Reuses all production logic, only overrides what's necessary

Minimal override: Only changes signature validation to always return true

Maintains accuracy: All gas measurements and execution paths remain identical to production

Deploy separately: Simulation contracts are deployed independently, keeping production contracts clean

This is much better than the previous approach of embedding simulation flags throughout the production code. The simulation still exercises the same code paths for accurate gas estimation.

github-actions · 2025-07-22T14:38:50Z

src/simulation/OrchestratorSim.sol

+    /// But the balance of msg.sender has to be equal to type(uint256).max, to prove that a state override has been made offchain,
+    /// and this is not an onchain call. This mode has been added so that receipt logs can be generated for `eth_simulateV1`
+    /// @return gasUsed The amount of gas used by the execution. (Only returned if `isStateOverride` is true)
+    function simulateExecute(bool isStateOverride, bytes calldata encodedIntent)


Well-Designed Simulation Interface

✅ Good preservation of simulation API while cleaning up production code!

Analysis:

Maintains backward compatibility: simulateExecute function preserved for existing tooling

Simplified implementation: Reuses main _execute logic without complex flag passing

Clear error handling: Maintains the SimulationPassed revert pattern for gas measurement

State override support: Still supports the isStateOverride parameter for eth_simulateV1

Minor suggestion: The combinedGasOverride parameter was removed from simulateExecute. If this breaks existing simulation callers, consider documenting this as a breaking change or adding backward compatibility.

github-actions · 2025-07-22T14:39:00Z

src/Orchestrator.sol

-    /// If `incremented` is true and `err` is non-zero, the Intent was successful.
-    /// For PreCalls where the nonce is skipped, this event will NOT be emitted..
-    event IntentExecuted(address indexed eoa, uint256 indexed nonce, bool incremented, bytes4 err);
+    /// - `nonce` denotes the nonce that has been incremented to invalidate `nonce`.


Event Simplification - Good Change

✅ Excellent simplification of the IntentExecuted event!

Improvements:

Clearer semantics: Event now only emits when nonce is successfully incremented

Removes confusing state: No more incremented and err fields that required complex interpretation

Simpler indexing: Easier for off-chain services to track successful intent executions

Consistent behavior: Event emission is now deterministic - only emitted on success

This aligns well with the simplified execute flow where failures result in reverts rather than error codes.

github-actions · 2025-07-22T14:39:08Z

src/Orchestrator.sol

-        Intent calldata i = _extractIntent(encodedIntent);
-
-        uint256 g = Math.coalesce(uint96(combinedGasOverride), i.combinedGas);
+    // TODO: do we need something like the combinedGasOverride anymore?


TODO Comment Needs Resolution

The TODO comment mentions combinedGasOverride which was part of the simulation logic. Since simulation has been moved to separate contracts, this TODO should be:

Resolved: If the gas override functionality is no longer needed

Updated: If there's still a need for gas override in production execution

Documented: If this relates to future functionality

Suggestion: Either remove this TODO or update it to reflect the current architecture where simulation happens in separate contracts.

github-actions · 2025-07-22T14:39:19Z

src/Orchestrator.sol

+            mstore(add(m, 0x40), digest) // Add digest as third param

            let encodedIntentLength := sub(calldatasize(), 0x24)
            // NOTE: The intent encoding here is non standard, because the data offset does not start from the beginning of the calldata.


Intent Encoding Standardization TODO

This TODO comment about non-standard intent encoding is important for maintainability:

// NOTE: The intent encoding here is non standard, because the data offset does not start from the beginning of the calldata. // The data offset starts from the location of the intent offset itself. The decoding is done accordingly in the receiving function. // TODO: Make the intent encoding standard.

Recommendation: Consider addressing this in a follow-up PR since:

Current risk: Low - the encoding/decoding is isolated to internal functions

Future maintenance: Standardizing would improve code clarity and reduce bugs

Gas impact: Should verify any standardization doesn't negatively impact gas usage

This refactor already simplifies the codebase significantly, so this TODO could be addressed separately.

howydev · 2025-08-08T21:08:04Z

src/Orchestrator.sol

 import {ISettler} from "./interfaces/ISettler.sol";
 import {MerkleProofLib} from "solady/utils/MerkleProofLib.sol";

+import "forge-std/console.sol";


nit

Suggested change

import "forge-std/console.sol";

howydev · 2025-08-08T21:16:44Z

src/Orchestrator.sol

-    ///
-    /// This function reverts if the PREP initialization or the Intent validation fails.
-    /// This is to prevent incorrect compensation (the Intent's signature defines what is correct).
-    function selfCallPayVerifyCall537021665() public payable {


good catch, i don't think we need verify to be in a separate call frame

howydev · 2025-08-08T21:17:31Z

src/Orchestrator.sol

+
        // Equivalent Solidity code:
-        // try this.selfCallExecutePay(flags, keyHash, i) {}
+        // try this.selfCallExecutePay( keyHash, i) {}


nit

Suggested change

// try this.selfCallExecutePay( keyHash, i) {}

// try this.selfCallExecutePay(keyHash, i) {}

howydev · 2025-08-08T21:17:36Z

src/Orchestrator.sol

        // off-chain simulation and on-chain execution.
        if (i.prePaymentAmount != 0) _pay(i.prePaymentAmount, keyHash, digest, i);

+        console.log("B EOA: ", i.eoa);


nit

Suggested change

console.log("B EOA: ", i.eoa);

howydev · 2025-08-08T21:21:56Z

src/Orchestrator.sol

+            mstore(add(m, 0x20), keyHash) // Add keyHash as second param
+            mstore(add(m, 0x40), digest) // Add digest as third param
+            mstore(add(m, 0x60), 0x80) // Add offset of encoded Intent as third param
+            let encodedIntentLength := sub(calldatasize(), 0x44)


this is potentially dangerous i think, a valid calldata for the top level execute might be:

fnSelector | offset | junk data | length | intent

then this would end up grabbing junk data

howydev · 2025-08-08T21:26:32Z

src/Orchestrator.sol

            }
        }
+
+        emit IntentExecuted(i.eoa, i.nonce);


have a slight preference to keep the success/failure or only conditionally emit this on call success, otherwise it might not provide very useful data if it also contains execution failures

howydev · 2025-08-08T21:29:06Z

src/Orchestrator.sol

+            0x44: 0x80 ( offset of encodedIntent)
+            0x64: encodedIntentLength
+            0x84: encodedIntent data
+                0x84: 0x20 ( offset of intent struct data, from starting of encodedIntent)


same as above, potentially risky to rely on this as the encoding

howydev · 2025-08-08T21:34:08Z

src/Orchestrator.sol

            // Event so that indexers can know that the nonce is used.
            // Reaching here means there's no error in the PreCall.
-            emit IntentExecuted(eoa, p.nonce, true, 0); // `incremented = true`, `err = 0`.
+            emit IntentExecuted(eoa, p.nonce);


should we differentiate between the precall events and the main intent events?

howydev · 2025-08-08T21:37:51Z

src/simulation/OrchestratorSim.sol

+        returns (uint256 /*gAccountExecute*/ )
+    {
+        // If Simulation fails, then it will revert here.
+        execute(encodedIntent);


iiuc this would be an internal call, so all the calldataX operations operate on the top level calldata that would include isRevert

legion2002 added 4 commits July 21, 2025 16:24

feat: removing batching from the orchestrator

b78b0c4

feat: add peripheral intent batching contract

dc628ec

fix: combined gas issue, use executeGas instead

6151a44

feat: create separate simulation contracts with bytecode overloads

478cde0

legion2002 marked this pull request as draft July 22, 2025 14:35

github-actions bot reviewed Jul 22, 2025

View reviewed changes

legion2002 added 7 commits July 22, 2025 20:37

chore: return gAccountExecute in OrchestratorSim

ed98d3a

chore: update simulator to new gas mechanism (WIP)

88eeb7d

chore: updated simulator

b9adc71

chore: (WIP) fixing tests

2cdc071

fix: intent encoding in self call standardized

5e2a016

fix: some overrides, still has a bug related to code overrides

0c22774

fix: bunch of fixes still WIP

0a5058b

howydev reviewed Aug 8, 2025

View reviewed changes

legion2002 added this to the v1.0.0 milestone Sep 11, 2025

legion2002 self-assigned this Sep 16, 2025

-            // remaining flow after the self call.
+            // Ensure sufficient gas for safe execution:
+            // - Prevents execution at dangerous call depth
+            // - Reserves gas for post-execution operations (payments, cleanup)
+            // - Uses 63/64 gas forwarding rule with safety buffer
+            if (((gasleft() * 63) >> 6) < Math.saturatingAdd(i.executeGas, _INNER_GAS_OVERHEAD)) {

	// try this.selfCallExecutePay( keyHash, i) {}
	// try this.selfCallExecutePay(keyHash, i) {}

Simplification & Fixes in Orchestrator and Account #258

Are you sure you want to change the base?

Simplification & Fixes in Orchestrator and Account #258

Uh oh!

Conversation

legion2002 commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ PR Review Complete

🎯 Key Findings

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Comprehensive Review: Orchestrator Simplification & Fixes

🎯 Key Achievements

🔒 Security Analysis

📊 Code Quality Assessment

🚀 Recommendations

💡 Overall Assessment

Uh oh!

Uh oh!

Uh oh!

github-actions bot Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

howydev Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

howydev Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

howydev Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

howydev Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

howydev Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

howydev Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

howydev Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

howydev Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

howydev Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

legion2002 commented Jul 22, 2025 •

edited

Loading

github-actions bot commented Jul 22, 2025 •

edited

Loading

howydev Aug 8, 2025 •

edited

Loading

howydev Aug 8, 2025 •

edited

Loading

howydev Aug 8, 2025 •

edited

Loading