Conversation
This commit increments the version number to 1.0.0-alpha.15 in the package.json files for both the contracts and SDK packages. This version update ensures consistency across the project and prepares the packages for the upcoming release cycle.
This commit introduces a new internal function, `_validateMetaTxAction`, to enforce strict validation of meta-transaction actions within the EngineBlox library. The function ensures that the expected signer action aligns with the current workflow for meta-transaction requests, approvals, and cancellations. Additionally, the existing meta-transaction functions `txCancellationWithMetaTx`, `txApprovalWithMetaTx`, and `txRequestAndApproveWithMetaTx` are updated to utilize this new validation, improving the security and integrity of the transaction handling process. These changes contribute to a more robust and maintainable codebase.
This commit introduces new function selectors for time-lock operations in the GuardController tests, specifically `executeWithTimeLock`, `approveTimeLockExecutionWithMetaTx`, and `cancelTimeLockExecutionWithMetaTx`. It updates the ERC20MintControllerTests to verify that the MINT_REQUESTOR and MINT_APPROVER roles have the appropriate permissions for these selectors. Additionally, the test output messages are refined for clarity regarding token minting and balance verification. These changes improve the accuracy and maintainability of the testing framework.
…library This commit updates the function schema in the BaseStateMachine contract and the EngineBlox library to include a new parameter, `enforceHandlerRelations`, which enforces strict alignment between handler selectors and schema definitions. The changes improve the clarity and functionality of the function schema, ensuring better validation of handler relationships during permission checks. Additionally, the documentation is updated to reflect these changes, enhancing the overall maintainability and robustness of the codebase.
…Machine This commit introduces improvements to the BaseStateMachine class by allowing callers to specify an explicit gas limit for large calldata payloads, enhancing flexibility in transaction execution. Additionally, it adds a new `simulationMode` option to control pre-flight simulation behavior, offering three modes: 'strict', 'warn-only', and 'skip'. This enhancement improves error handling and user experience by providing more control over transaction simulations, ensuring better feedback during contract interactions. The documentation for the `TransactionOptions` interface is also updated to reflect these changes.
…itions This commit introduces a new boolean parameter, `enforceHandlerRelations`, to various ABI definitions across multiple contracts, including AccountBlox, BaseStateMachine, CopyBlox, and others. This enhancement enforces strict alignment between handler selectors and schema definitions, improving validation during permission checks. The changes ensure better clarity and functionality in the contract interactions, contributing to a more robust and maintainable codebase. Additionally, relevant documentation is updated to reflect these modifications.
This commit introduces a new feature in the Sanity SDK test runner that allows users to specify the number of sequential test runs using the `--repeat N` option, defaulting to 1. The implementation includes updates to the usage instructions and modifies the test execution logic to accommodate multiple runs. This enhancement improves the flexibility and usability of the testing framework, enabling more thorough testing scenarios. Additionally, relevant documentation is updated to reflect these changes.
Contributor
📝 WalkthroughWalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant Client
participant EngineBlox
participant PermissionValidator
participant SchemaRegistry
rect rgba(100, 200, 100, 0.5)
Note over Client,SchemaRegistry: Function Registration with enforceHandlerRelations
Client->>EngineBlox: registerFunction(..., enforceHandlerRelations, ...)
EngineBlox->>SchemaRegistry: Store FunctionSchema with enforceHandlerRelations flag
SchemaRegistry-->>EngineBlox: Schema registered
end
rect rgba(100, 150, 200, 0.5)
Note over Client,SchemaRegistry: Permission Validation Flow
Client->>PermissionValidator: validateHandlerRelations(handlerForSelectors, schema)
alt enforceHandlerRelations == true
PermissionValidator->>SchemaRegistry: Fetch schema.handlerForSelectors
SchemaRegistry-->>PermissionValidator: Return expected handlers
PermissionValidator->>PermissionValidator: Validate each handler exists in schema
alt Mismatch found
PermissionValidator-->>Client: Revert(HandlerForSelectorMismatch)
else All match
PermissionValidator-->>Client: Validation passed
end
else enforceHandlerRelations == false
PermissionValidator-->>Client: Skip validation (short-circuit)
end
end
rect rgba(200, 100, 100, 0.5)
Note over Client,SchemaRegistry: Meta-Transaction Validation
Client->>EngineBlox: txApprovalWithMetaTx(metaTxPayload, signature)
EngineBlox->>EngineBlox: _validateMetaTxAction(expectedAction)
alt Action matches expected
EngineBlox-->>Client: Proceed with execution
else Action mismatch
EngineBlox-->>Client: Revert(InvalidMetaTxAction)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
scripts/sanity/guard-controller/erc20-mint-controller-tests.cjs (3)
96-101:⚠️ Potential issue | 🟠 MajorVerify wallet membership before treating Step 1 as idempotent.
These branches pass Step 1 after calling
verifyStep1RolesAfter(), but that helper only checks that the roles exist. If the roles already exist with different members, this step still goes green and the failure is deferred to the later meta-tx path.💡 Minimal diff
async verifyStep1RolesAfter() { - const requestorExists = await this.roleExists(this.getRoleHash('MINT_REQUESTOR')); - const approverExists = await this.roleExists(this.getRoleHash('MINT_APPROVER')); + const requestorRoleHash = this.getRoleHash('MINT_REQUESTOR'); + const approverRoleHash = this.getRoleHash('MINT_APPROVER'); + const requestorExists = await this.roleExists(requestorRoleHash); + const approverExists = await this.roleExists(approverRoleHash); this.assertTest(requestorExists, 'MINT_REQUESTOR role exists after step 1'); this.assertTest(approverExists, 'MINT_APPROVER role exists after step 1'); + + const [requestorAssigned, approverAssigned] = await Promise.all([ + this.contract.methods.hasRole(requestorRoleHash, this.mintRequestorWallet.address).call(), + this.contract.methods.hasRole(approverRoleHash, this.mintApproverWallet.address).call() + ]); + this.assertTest(requestorAssigned, `MINT_REQUESTOR contains ${this.mintRequestorWallet.address}`); + this.assertTest(approverAssigned, `MINT_APPROVER contains ${this.mintApproverWallet.address}`); }Also applies to: 137-140, 148-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity/guard-controller/erc20-mint-controller-tests.cjs` around lines 96 - 101, The current idempotency branch checks only that roles exist (using roleExists/getRoleHash and verifyStep1RolesAfter) but not that the role members match expected wallets, so Step 1 can falsely pass; update the branch in the handler that currently calls verifyStep1RolesAfter() and passTest('Create MINT_REQUESTOR and MINT_APPROVER roles', ...) to also fetch and compare the current role members to the expected wallet addresses (use whatever helper exists for reading role members or add one), and only treat the step as satisfied if both roles exist and their members exactly match the expected set; if membership differs, proceed with the creation flow (or fail) instead of passing. Ensure the same membership check is added to the other similar branches around the lines that call verifyStep1RolesAfter() (the ones referenced near lines 137-140 and 148-152).
545-555:⚠️ Potential issue | 🟠 MajorThis still exercises the legacy one-step handler instead of the timelock approval path.
Step 5 signs
SIGN_META_REQUEST_AND_APPROVEand sendsrequestAndApproveExecution(...), so it never hitsexecuteWithTimeLockorapproveTimeLockExecutionWithMetaTx. The new controller permissions added above can regress without this test catching it.Also applies to: 597-604
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity/guard-controller/erc20-mint-controller-tests.cjs` around lines 545 - 555, The test currently uses SIGN_META_REQUEST_AND_APPROVE and calls requestAndApproveExecution, which exercises the legacy one-step handler; change it to create a request-only meta-tx (use SIGN_META_REQUEST or the equivalent constant in tests when calling createExternalExecutionMetaTx) and then call the requestExecution flow instead of requestAndApproveExecution so the code hits the timelock path; after requesting, simulate the approver's timelock approval by invoking approveTimeLockExecutionWithMetaTx (or building an approve meta-tx signed by mintApproverWallet and calling executeWithTimeLock) so the test exercises executeWithTimeLock and approveTimeLockExecutionWithMetaTx rather than the one-step handler.
256-314:⚠️ Potential issue | 🟠 MajorStep 4 can report success without all of the permissions this flow uses.
The idempotent gate and the final verification still miss several permissions configured below: the handler-side
SIGN_META_REQUEST_AND_APPROVE, the broadcaster'sREQUEST_AND_APPROVE_EXECUTION_SELECTORpermission, and the broadcaster's timelock approve/cancel controller permissions are never all asserted. That makes reruns prone to false positives.Also applies to: 457-502
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity/guard-controller/erc20-mint-controller-tests.cjs` around lines 256 - 314, The final success gate is missing checks for the handler-side SIGN_META_REQUEST_AND_APPROVE and several broadcaster permissions; add roleHasPermissionForSelector calls to assert: (1) the handler/approver role (handlerRoleHash or approverRoleHash as appropriate) has SIGN_META_REQUEST_AND_APPROVE on REQUEST_AND_APPROVE_EXECUTION_SELECTOR, (2) the broadcasterRoleHash has REQUEST_AND_APPROVE_EXECUTION_SELECTOR (with the correct TxAction), and (3) the broadcasterRoleHash has APPROVE_TIMELOCK_EXECUTION_META_SELECTOR and CANCEL_TIMELOCK_EXECUTION_META_SELECTOR with the appropriate timelock TxAction; store each result in a descriptive const (e.g. handlerHasSignMetaRequestAndApprove, broadcasterHasHandlerSelector, broadcasterHasApproveMetaTx, broadcasterHasCancelMetaTx) and include those consts in the big if(...) alongside the existing requestorOk/.../broadcasterOk checks so the idempotent gate only reports success when all permissions are present (use roleHasPermissionForSelector to perform each check).test/foundry/helpers/PaymentTestHelper.sol (1)
185-198:⚠️ Potential issue | 🟡 Minor
approveTransactionis missing the selector it now claims to enforce.
requestTransaction*were updated to advertisenativeTransferSelector, butapproveTxHandlersstill only containsapproveTxSelector. WithenforceHandlerRelations = true, approvals for theNATIVE_TRANSFER_SELECTORrequests this helper creates can now fail the handler-to-selector check. Either addnativeTransferSelectorhere or disable relation enforcement for this generic helper.Suggested fix
- bytes4[] memory approveTxHandlers = new bytes4[](1); - approveTxHandlers[0] = approveTxSelector; // Self-reference + bytes4[] memory approveTxHandlers = new bytes4[](2); + approveTxHandlers[0] = approveTxSelector; // Self-reference + approveTxHandlers[1] = nativeTransferSelector; // Approves NATIVE_TRANSFER_SELECTOR in payment tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/foundry/helpers/PaymentTestHelper.sol` around lines 185 - 198, The approveTransaction registration is enforcing handler relations but approveTxHandlers only contains approveTxSelector, so handlers won't match requests that advertise nativeTransferSelector; update the register call in the block that builds approveTxHandlers used by EngineBlox.registerFunction: either expand approveTxHandlers to length 2 and push nativeTransferSelector alongside approveTxSelector (ensure you reference nativeTransferSelector and approveTxHandlers) or set enforceHandlerRelations to false in the EngineBlox.registerFunction call; make the minimal change consistent with the helper's intended behavior (prefer adding nativeTransferSelector if approvals should cover native transfers).contracts/core/execution/lib/definitions/GuardControllerDefinitions.sol (1)
224-247:⚠️ Potential issue | 🟠 MajorThe
REGISTER_FUNCTIONaction must includeenforceHandlerRelationsto support dynamic registration.Static schemas now support the
enforceHandlerRelationsboolean (e.g., schema[6] enables it at line 231), but the runtime registration payload does not. TheencodeRegisterFunction()helper andgetGuardConfigActionSpecs()format specification documentREGISTER_FUNCTIONas(string functionSignature, string operationName, TxAction[] supportedActions)only — noenforceHandlerRelationsfield. Functions registered dynamically viaguardConfigBatchcannot set the new validation level and will silently default to weaker behavior.Evidence
- Static schema (line 231):
schemas[6]setsenforceHandlerRelations: true- Runtime encoder (lines 479–485):
encodeRegisterFunction()takes onlyfunctionSignature,operationName,supportedActions- Format spec (line 445): REGISTER_FUNCTION format is
"(string functionSignature, string operationName, TxAction[] supportedActions)"— no boolean- Schema struct (EngineBlox.sol line 155):
FunctionSchemahasbool enforceHandlerRelations- Runtime enforcement (EngineBlox.sol line 2165): The check
if (!schema.enforceHandlerRelations)confirms the field is actively used during execution🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/core/execution/lib/definitions/GuardControllerDefinitions.sol` around lines 224 - 247, The runtime REGISTER_FUNCTION payload must include the enforceHandlerRelations boolean so dynamically registered functions can set the same validation level as static schemas; update the REGISTER_FUNCTION format spec and the encoder/decoder to carry this field: modify the format string used in getGuardConfigActionSpecs() and any spec docs from "(string functionSignature, string operationName, TxAction[] supportedActions)" to "(string functionSignature, string operationName, TxAction[] supportedActions, bool enforceHandlerRelations)", then update encodeRegisterFunction() to accept and encode the enforceHandlerRelations bool into the registration tuple and ensure the runtime that constructs EngineBlox.FunctionSchema uses this value to populate FunctionSchema.enforceHandlerRelations so the enforcement logic (EngineBlox.FunctionSchema.enforceHandlerRelations) behaves identically for dynamic registrations.scripts/sanity-sdk/runtime-rbac/rbac-tests.ts (1)
1108-1170:⚠️ Potential issue | 🟠 MajorDon't let
HandlerForSelectorMismatchturn this helper into best-effort mode.
ensureRoleHasRequiredPermissions()is the setup gate for the rest of the RBAC workflow. This branch now returns even when only one required permission is present—or when neither can be verified—so Step 1 can pass and later steps fail with much harder-to-diagnose authorization errors.💡 Suggested fix
- } else if (isHandlerMismatch && (verifyHandler || verifyExecution)) { - console.log(` ✅ Permissions partially or fully present after HandlerForSelectorMismatch (re-run safe)`); - } else if (isHandlerMismatch) { - console.log(` ⚠️ HandlerForSelectorMismatch and permissions not found; continuing (may need contract fix for re-runs)`); + } else if (isHandlerMismatch) { + if (verifyHandler && verifyExecution) { + console.log(` ✅ Permissions already present after HandlerForSelectorMismatch (re-run safe)`); + } else { + throw new Error( + `HandlerForSelectorMismatch left required permissions missing. handler=${verifyHandler}, execution=${verifyExecution}` + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity-sdk/runtime-rbac/rbac-tests.ts` around lines 1108 - 1170, The helper ensureRoleHasRequiredPermissions currently treats HandlerForSelectorMismatch as an acceptable outcome even when only one or neither permission is verified; change it to require both verifyHandler and verifyExecution to be true before proceeding. After the retry loop, remove the early-success branches that allow partial success for isHandlerMismatch/isResourceAlreadyExists; instead: if verifyHandler && verifyExecution, continue normally; else if isResourceAlreadyExists or isHandlerMismatch but verification failed, throw a clear Error from ensureRoleHasRequiredPermissions (include txStatus.error and roleHash) so the setup gate fails fast; keep the special-case logging when verify succeeds to allow re-runs, but do not let HandlerForSelectorMismatch turn this into best-effort mode. Ensure references: isHandlerMismatch, isResourceAlreadyExists, verifyHandler, verifyExecution, ROLE_CONFIG_BATCH_META_SELECTOR, ROLE_CONFIG_BATCH_EXECUTE_SELECTOR, TxAction, and function ensureRoleHasRequiredPermissions.contracts/core/base/BaseStateMachine.sol (1)
702-720:⚠️ Potential issue | 🔴 CriticalThis is a storage layout violation in an upgradeable contract that could break existing proxy deployments.
FunctionSchemais persisted as a mapping in_secureState(EngineBlox.sol:179). The two bool fieldsenforceHandlerRelations(line 154) andisProtected(line 155) are packed into the same storage slot. IfenforceHandlerRelationswas added as a new field, existing proxies would haveisProtectedbits stored at theenforceHandlerRelationsposition, causing misinterpretation on read.The contract is upgradeable (extends
Initializable,ERC165Upgradeable,ReentrancyGuardUpgradeable), yet there are no__gaparrays or storage layout preservation mechanisms. If this field is being introduced to an existing system with deployed proxies, a migration strategy must be implemented before the upgrade is deployed.Per coding guidelines: "Preserve storage layout using __gap arrays in upgradeable contracts."
Also applies to: lines 885-893 (
_loadDefinitionscalls_registerFunctionwith the new field)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/core/base/BaseStateMachine.sol` around lines 702 - 720, The new boolean field packing introduced into FunctionSchema causes a storage layout break for upgradeable proxies; update the upgrade to preserve layout by adding a storage gap (e.g., __gap) to BaseStateMachine (or the contract that defines FunctionSchema/_secureState) and implement an explicit migration routine that safely initializes/moves the existing mapping data before deploying the change; locate references to FunctionSchema, _secureState, _registerFunction and _loadDefinitions and either (a) add reserved uint256[] __gap slots to the contract defining persistent storage so the new fields do not shift existing slots, or (b) provide a migration function that reads old packed values and writes them into the new schema positions prior to using _registerFunction/_loadDefinitions in the upgraded implementation.contracts/core/lib/EngineBlox.sol (2)
149-158:⚠️ Potential issue | 🟠 MajorThis field insertion is not storage-safe for in-place upgrades.
FunctionSchema.supportedActionsBitmapandisProtectedwere previously packed into the same slot. InsertingenforceHandlerRelationsbetween them means oldisProtected=trueentries will now be read back asenforceHandlerRelations=trueandisProtected=false, which can silently unprotect previously protected schemas. Either append the new flag at the end of the struct or ship a migration that rewrites every stored schema before upgrade.As per coding guidelines, "Preserve storage layout using __gap arrays in upgradeable contracts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/core/lib/EngineBlox.sol` around lines 149 - 158, The insertion of enforceHandlerRelations into FunctionSchema between supportedActionsBitmap and isProtected breaks existing storage packing and will misread old data; to fix, move the new boolean enforceHandlerRelations to the end of the struct (after handlerForSelectors) or implement a migration that reads every stored FunctionSchema and rewrites it with the new layout; update any code that constructs or reads FunctionSchema (references: struct FunctionSchema, fields supportedActionsBitmap, enforceHandlerRelations, isProtected, handlerForSelectors) to match the new layout and ensure upgrade-safe storage (or add an explicit migration step before activating the new contract).
479-486:⚠️ Potential issue | 🔴 CriticalThe new action check still leaves handler and transaction binding attacker-controlled.
These paths now pin
metaTx.params.action, butrequestAndApprovestill trusts caller-suppliedmetaTx.params.handlerSelector/handlerContract, and the approval/cancel flows authorize againstmetaTx.txRecord.params.executionSelectorfrom calldata even though they executeself.txRecords[txId]. A signer can therefore satisfy RBAC with one signed handler/selector and use it to drive a different entrypoint or pending transaction. Require the signed handler to match the current contract and entrypoint, and for existing-tx flows compare the signed params againstself.txRecords[txId](or rebuild the signed hash from storage) before permission checks and signature verification.As per coding guidelines, "Validate EIP-712 signatures for meta-transactions before execution".
Also applies to: 500-506, 541-546, 1626-1657, 2234-2240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/core/lib/EngineBlox.sol` around lines 479 - 486, The meta-tx flows (e.g., txCancellationWithMetaTx and requestAndApprove) currently trust caller-provided metaTx.params.handlerSelector/handlerContract and metaTx.txRecord.params.executionSelector from calldata, enabling signer mismatch attacks; fix by first comparing the signed handler fields to the actual handler and selector the contract will use (require metaTx.params.handlerSelector == address(this).selector-equivalent or the actual entrypoint selector and metaTx.params.handlerContract == address(this) as appropriate), and for flows that operate on an existing txId (txCancellationWithMetaTx, approve/cancel flows) require that metaTx.txRecord.params.executionSelector exactly matches self.txRecords[txId].params.executionSelector (or rebuild the signed hash from storage and validate equality) before calling _validateExecutionAndHandlerPermissions or verifySignature; update verifySignature/_validateExecutionAndHandlerPermissions call sites to perform these equality checks first so signature and RBAC checks are applied to the actual stored target, not attacker-supplied calldata.scripts/sanity-sdk/guard-controller/erc20-mint-controller-tests.ts (2)
66-82:⚠️ Potential issue | 🟡 MinorThe wrong-action regression test is currently inert.
executeTests()never callsstep2aMintMetaTxWrongActionMustRevert(), and inside that helper any revert counts as success. That means the newNotSupportedguard can regress without this suite noticing.Minimal fix
await this.step0RegisterMintSchemaIfNeeded(); await this.step1WhitelistBasicErc20IfNeeded(); await this.step1bEnsureMintRolesAndPermissions(); + await this.step2aMintMetaTxWrongActionMustRevert(); await this.step2Mint100ToAccountBloxViaMetaTx(); await this.step3VerifyBalanceIncrease(); await this.step4SnapshotMintReadinessState();Please also assert that the decoded revert is
NotSupported, not just that something threw.Also applies to: 581-661
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity-sdk/guard-controller/erc20-mint-controller-tests.ts` around lines 66 - 82, The test suite never invokes step2aMintMetaTxWrongActionMustRevert() from executeTests(), and the helper currently treats any revert as success; update executeTests() to call step2aMintMetaTxWrongActionMustRevert() in the correct sequence (after step2Mint100ToAccountBloxViaMetaTx or where the wrong-action flow belongs) and modify step2aMintMetaTxWrongActionMustRevert() to assert that the transaction reverts with the specific decoded error name "NotSupported" (decode the revert payload and assert equality rather than accepting any thrown error) so the test fails on regressions to the NotSupported guard.
901-948:⚠️ Potential issue | 🟠 MajorThrow on unexpected byte fields instead of silently returning non-string values.
The
toHexhelper declares return typestringbut can return non-string values (numbers, objects) due to the type cast not enforcing runtime behavior. Only aconsole.warnis logged. Since this helper runs immediately before signing and sending, malformed inputs should throw rather than corrupt the meta-transaction payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity-sdk/guard-controller/erc20-mint-controller-tests.ts` around lines 901 - 948, The toHex helper inside normalizeMetaTxToHex can return non-string values and only logs a warning; change it to validate inputs and throw on unexpected types so it always returns a proper hex string. Specifically, update normalizeMetaTxToHex -> toHex to: accept string/Uint8Array (and strings starting with 0x), convert Uint8Array via bytesToHex, verify the result matches /^0x[0-9a-fA-F]*$/ (or pad/normalize as needed), and throw an Error for any other type or malformed value instead of returning '0x' or logging; ensure callers (message, signature, data, txRecord.message, params.executionSelector, params.operationType) rely on this invariant and adjust any fallback logic to avoid silent non-string returns.
🧹 Nitpick comments (5)
scripts/sanity/guard-controller/erc20-mint-controller-tests.cjs (1)
341-387: Split broadcaster approve/cancel rights per selector.Granting both
EXECUTE_META_APPROVEandEXECUTE_META_CANCELto both timelock controller selectors is broader than needed and makes the test less sensitive to selector/action mixups.♻️ Minimal diff
- const broadcasterApproveCancelActions = [this.TxAction.EXECUTE_META_APPROVE, this.TxAction.EXECUTE_META_CANCEL]; + const broadcasterApproveActions = [this.TxAction.EXECUTE_META_APPROVE]; + const broadcasterCancelActions = [this.TxAction.EXECUTE_META_CANCEL]; @@ - broadcasterApproveCancelActions, + broadcasterApproveActions, @@ - broadcasterApproveCancelActions, + broadcasterCancelActions,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity/guard-controller/erc20-mint-controller-tests.cjs` around lines 341 - 387, The test currently grants both EXECUTE_META_APPROVE and EXECUTE_META_CANCEL (broadcasterApproveCancelActions) to both APPROVE_TIMELOCK_EXECUTION_META_SELECTOR and CANCEL_TIMELOCK_EXECUTION_META_SELECTOR, which is too broad; update the calls that use addFunctionToRole for this.getRoleHash('BROADCASTER_ROLE') so APPROVE_TIMELOCK_EXECUTION_META_SELECTOR receives only EXECUTE_META_APPROVE and CANCEL_TIMELOCK_EXECUTION_META_SELECTOR receives only EXECUTE_META_CANCEL (create two distinct action arrays or inline single-action arrays instead of broadcasterApproveCancelActions), keeping the other addFunctionToRoleWithHandlerForSelectors/addFunctionToRole calls and role hashes (getRoleHash('BROADCASTER_ROLE'), REQUEST_AND_APPROVE_EXECUTION_SELECTOR, ERC20_MINT_SELECTOR) unchanged.sdk/typescript/abi/IDefinition.abi.json (1)
33-41: Ship this tuple expansion as a breaking SDK change.Inserting
enforceHandlerRelationsbeforeisProtectedshifts every positionalFunctionSchemafield aftersupportedActionsBitmap. Please verify all generated bindings and any consumers that destructure these tuples positionally were regenerated, and version the published SDK accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/typescript/abi/IDefinition.abi.json` around lines 33 - 41, The insertion of the new tuple field enforceHandlerRelations before isProtected in the ABI shifts positional indexes for FunctionSchema fields after supportedActionsBitmap; regenerate all generated TypeScript bindings that consume IDefinition.abi (so FunctionSchema-related types/parsers), update any consumers that destructure these tuples positionally to use named accessors or the new index order (referencing enforceHandlerRelations, isProtected, supportedActionsBitmap, FunctionSchema), and publish this as a breaking SDK release with an appropriate major version bump so downstream users are aware of the change.test/foundry/fuzz/UnregisterFunctionFuzz.t.sol (1)
39-47: Cover bothenforceHandlerRelationsmodes in this fuzz harness.The harness now hardcodes
true, so this suite still doesn't exercise the disabled path or any handler/selector mismatch edge cases introduced by the new flag. Please fuzz both boolean values and at least one mismatched-handler case, or move this to a regular unit test if it is meant to stay deterministic. As per coding guidelines,**/*.t.sol: Write fuzz tests for input validation and edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/foundry/fuzz/UnregisterFunctionFuzz.t.sol` around lines 39 - 47, The harness currently calls EngineBlox.registerFunction with enforceHandlerRelations hardcoded to true; change the fuzz input to include a boolean for enforceHandlerRelations (fuzz both true and false) and add at least one branch that constructs a mismatched handlers vs selectors scenario to exercise the disabled-path edge cases (e.g., provide a handlers array that does not align with functionSelector/bitmap) so both validation paths run; update the call site in UnregisterFunctionFuzz.t.sol where EngineBlox.registerFunction(state, functionSignature, functionSelector, "TEST_OPERATION", bitmap, true, false, handlers) is invoked to use the new fuzzed boolean and include a seeded mismatched-case path for handlers when the flag is false.test/foundry/fuzz/ComprehensiveDefinitionSecurityFuzz.t.sol (1)
355-388: This "fuzz" case never uses the fuzzed input.
handlerSelectornever feeds the schema or permission under test, so the runner repeats the same constant scenario on every iteration. Either make this a regular unit test or build the malicious fixture from the fuzzed selector.As per coding guidelines, "
**/*.t.sol: Write fuzz tests for input validation and edge cases."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/foundry/fuzz/ComprehensiveDefinitionSecurityFuzz.t.sol` around lines 355 - 388, The fuzz input handlerSelector is never used—testFuzz_DefinitionWithSelfReferenceBehavior always exercises the same constant fixture (MaliciousDefinitions_InvalidSelfReference.HANDLER_SELECTOR); update the test to use the fuzzed handlerSelector by either (A) passing handlerSelector into the malicious fixture helpers (replace calls to MaliciousDefinitions_InvalidSelfReference.getRolePermissions() and .getFunctionSchemas() with variants that accept handlerSelector) or (B) build the EngineBlox.FunctionSchema[] and IDefinition.RolePermission structs inline using handlerSelector instead of MaliciousDefinitions_InvalidSelfReference.HANDLER_SELECTOR before calling testStateMachine.loadDefinitionsForTesting, so the fuzz input actually affects the schema/permission and triggers different validation paths in testFuzz_DefinitionWithSelfReferenceBehavior.scripts/sanity-sdk/guard-controller/base-test.ts (1)
199-202: Consider hardening ABI param serialization to handle potentialbigintvalues.While the current implementation of
extractErrorInfo()converts all decoded parameters to strings (line 1032 insdk/typescript/utils/contract-errors.ts), theparamsfield is typed asRecord<string, any>, allowing for future code paths that might introduce unconvertedbigintvalues from ABI decoding.JSON.stringifywill throw if it encounters abigint, masking the original revert and turning the diagnostic message into the failure point.The suggested fix adds a replacer function to safely serialize numeric types:
💡 Suggested hardening
- console.error(` 📋 Revert decoded: ${decoded.name}${decoded.params ? ` ${JSON.stringify(decoded.params)}` : ''}`); + const decodedParams = decoded.params + ? JSON.stringify(decoded.params, (_, value) => + typeof value === 'bigint' ? value.toString() : value + ) + : ''; + console.error(` 📋 Revert decoded: ${decoded.name}${decodedParams ? ` ${decodedParams}` : ''}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity-sdk/guard-controller/base-test.ts` around lines 199 - 202, The revert logging can throw if decoded.params contains bigint values because JSON.stringify fails on bigints; update the code that logs and concatenates decoded.params (the use of decoded.params in the block using extractErrorInfo and the console.error/ hint assembly) to serialize params with a safe replacer that converts bigint (and other non-serializable numeric types) to strings before JSON.stringify. Specifically, change the serialization of decoded.params in the console.error and when building hint so it uses a JSON.stringify call with a replacer that returns value.toString() for typeof value === 'bigint' (and otherwise returns the value), ensuring no uncaught TypeError from JSON.stringify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@abi/CopyBlox.abi.json`:
- Around line 923-927: You inserted the new boolean enforceHandlerRelations into
the middle of the FunctionSchema tuple (before isProtected), which changes
positional ABI layout and will break positional decoders; to fix, move the
enforceHandlerRelations field to the end of the FunctionSchema tuple (or
alternatively only introduce it as part of an explicit
breaking-version/migration), updating the ABI so the original order of existing
fields (including isProtected) is preserved; reference the FunctionSchema tuple
and the fields enforceHandlerRelations and isProtected when making the change.
In `@abi/GuardControllerDefinitions.abi.json`:
- Around line 163-167: The helper ABI must accept and pass the new
enforceHandlerRelations flag: update the helper signature for
encodeRegisterFunction (currently
encodeRegisterFunction(string,string,TxAction[])) to include a boolean parameter
for enforceHandlerRelations, thread that boolean into the payload construction
where getFunctionSchemas/enforceHandlerRelations is set, and regenerate the ABI
and SDK artifacts so callers can supply the flag; ensure all usages of
encodeRegisterFunction, any wrapper helpers, and tests are updated to pass the
new boolean.
In `@abi/SimpleVault.abi.json`:
- Around line 1045-1049: The ABI change inserted the new boolean field
enforceHandlerRelations before isProtected, which alters the positional tuple
layout of EngineBlox.FunctionSchema and will break any consumer decoding
getFunctionSchema() by index; restore compatibility by moving
enforceHandlerRelations to the end of the struct/ABI ordering (or explicitly
mark this as a breaking change and bump the contract/ABI version), update any
consumers to use named decoding instead of index-based access, and ensure the
final ABI keeps the original field order (isProtected in its original position)
unless you deliberately release a breaking version.
In
`@contracts/examples/integrations/Safe/GuardianSafe/GuardianSafeDefinitions.sol`:
- Around line 97-98: The cancelTransaction(uint256) path is not authorized in
the execution-selector layer when enforceHandlerRelations is true; update the
permissions so the handler and the linked EXEC_SAFE_TX_SELECTOR agree: add
EngineBlox.TxAction.EXECUTE_TIME_DELAY_CANCEL to executionActions and to the
owner's EXEC_SAFE_TX_SELECTOR permission set (and any other owner/handler
permission mappings touched by enforceHandlerRelations) so cancelTransaction is
authorized end-to-end when enforceHandlerRelations and isProtected are enabled.
In `@scripts/sanity-sdk/run-all-tests.ts`:
- Around line 171-188: The post-sanity system state summary is being executed
unconditionally; guard the call to printPostSanitySystemStateSummary so it only
runs when allPassed is true (i.e., the all-green case). Move the try/catch that
calls this.printPostSanitySystemStateSummary(testsToRun) inside the if
(allPassed) branch (or wrap it with if (allPassed) { ... }) and keep the
existing process.exit(allPassed ? 0 : 1) behavior unchanged so failures do not
trigger the post-sanity dump.
- Around line 294-302: The current --repeat handling removes the next token
(args.splice) before validating it, so malformed values like "--repeat
--guard-controller" or non-numeric strings get stripped and silently ignored;
update the logic in the repeat parsing block (look for repeatIdx, parseInt,
Number.isInteger, this.repeatCount, args.splice) to validate the next token
exists and is a valid integer in range before mutating args: if repeatIdx >= 0
then first check args[repeatIdx + 1] is present and parseInt yields a
Number.isInteger(n) within 1..100; only then set this.repeatCount and call
args.splice(repeatIdx, 2); otherwise throw or log an error and exit (or leave
args untouched) to prevent removing the wrong token.
In `@sdk/typescript/abi/BaseStateMachine.abi.json`:
- Around line 1144-1148: The ABI change inserted the new boolean field
"enforceHandlerRelations" into the middle of EngineBlox.FunctionSchema, which
shifts tuple indexes returned by getFunctionSchema(); to fix this, move the
"enforceHandlerRelations" entry to the end of the EngineBlox.FunctionSchema
struct in BaseStateMachine.abi.json so existing positional decoding remains
stable, or if intentional, mark and release the ABI as a breaking change and
document that getFunctionSchema() tuple indexes have changed.
In `@sdk/typescript/abi/RuntimeRBAC.abi.json`:
- Around line 861-865: The new bool field enforceHandlerRelations was inserted
before isProtected in the public tuple EngineBlox.FunctionSchema, shifting all
subsequent tuple indices and breaking positional decoders; fix by moving
enforceHandlerRelations to the end of the FunctionSchema tuple layout (append
after all existing fields) so indices for existing fields remain stable, or if
you must keep the new position, treat it as a breaking ABI change by bumping the
ABI/contract version and update any positional decoders,
serializers/deserializers and tests that reference EngineBlox.FunctionSchema or
the isProtected index accordingly.
In `@sdk/typescript/abi/RuntimeRBACDefinitions.abi.json`:
- Around line 72-76: The ABI change inserted "enforceHandlerRelations" into the
middle of EngineBlox.FunctionSchema and shifted subsequent tuple indexes
(including "isProtected"), which breaks compatibility; move
"enforceHandlerRelations" to the end of the FunctionSchema tuple instead of
inserting it in the middle and then regenerate the ABI
(RuntimeRBACDefinitions.abi.json) so the tuple order preserves existing indexes
and consumers remain compatible.
In `@sdk/typescript/abi/SimpleRWA20Definitions.abi.json`:
- Around line 111-115: The ABI insertion of "enforceHandlerRelations" before
"isProtected" breaks positional decoding for getFunctionSchemas() which returns
an array of EngineBlox.FunctionSchema; either move the "enforceHandlerRelations"
entry to the end of the function schema object array (append it after existing
fields) to preserve existing field positions, or treat this as an explicit
breaking change by bumping the ABI/schema version and documenting the reorder;
locate the "enforceHandlerRelations" field in SimpleRWA20Definitions.abi.json
and adjust its position (or version) so decoders relying on
EngineBlox.FunctionSchema maintain correct positional mappings.
In `@sdk/typescript/abi/SimpleVault.abi.json`:
- Around line 1045-1049: The ABI change inserted the new boolean field
enforceHandlerRelations before isProtected, altering the positional tuple layout
used by EngineBlox.FunctionSchema and breaking clients that index tuple members;
to fix, move enforceHandlerRelations to the end of the return tuple (append it
after existing fields) in SimpleVault.abi.json so existing positional
deserialization remains stable, or if deliberate, mark this as an explicit
breaking ABI change in the SDK release notes and bump the ABI version referenced
by EngineBlox.FunctionSchema.
---
Outside diff comments:
In `@contracts/core/base/BaseStateMachine.sol`:
- Around line 702-720: The new boolean field packing introduced into
FunctionSchema causes a storage layout break for upgradeable proxies; update the
upgrade to preserve layout by adding a storage gap (e.g., __gap) to
BaseStateMachine (or the contract that defines FunctionSchema/_secureState) and
implement an explicit migration routine that safely initializes/moves the
existing mapping data before deploying the change; locate references to
FunctionSchema, _secureState, _registerFunction and _loadDefinitions and either
(a) add reserved uint256[] __gap slots to the contract defining persistent
storage so the new fields do not shift existing slots, or (b) provide a
migration function that reads old packed values and writes them into the new
schema positions prior to using _registerFunction/_loadDefinitions in the
upgraded implementation.
In `@contracts/core/execution/lib/definitions/GuardControllerDefinitions.sol`:
- Around line 224-247: The runtime REGISTER_FUNCTION payload must include the
enforceHandlerRelations boolean so dynamically registered functions can set the
same validation level as static schemas; update the REGISTER_FUNCTION format
spec and the encoder/decoder to carry this field: modify the format string used
in getGuardConfigActionSpecs() and any spec docs from "(string
functionSignature, string operationName, TxAction[] supportedActions)" to
"(string functionSignature, string operationName, TxAction[] supportedActions,
bool enforceHandlerRelations)", then update encodeRegisterFunction() to accept
and encode the enforceHandlerRelations bool into the registration tuple and
ensure the runtime that constructs EngineBlox.FunctionSchema uses this value to
populate FunctionSchema.enforceHandlerRelations so the enforcement logic
(EngineBlox.FunctionSchema.enforceHandlerRelations) behaves identically for
dynamic registrations.
In `@contracts/core/lib/EngineBlox.sol`:
- Around line 149-158: The insertion of enforceHandlerRelations into
FunctionSchema between supportedActionsBitmap and isProtected breaks existing
storage packing and will misread old data; to fix, move the new boolean
enforceHandlerRelations to the end of the struct (after handlerForSelectors) or
implement a migration that reads every stored FunctionSchema and rewrites it
with the new layout; update any code that constructs or reads FunctionSchema
(references: struct FunctionSchema, fields supportedActionsBitmap,
enforceHandlerRelations, isProtected, handlerForSelectors) to match the new
layout and ensure upgrade-safe storage (or add an explicit migration step before
activating the new contract).
- Around line 479-486: The meta-tx flows (e.g., txCancellationWithMetaTx and
requestAndApprove) currently trust caller-provided
metaTx.params.handlerSelector/handlerContract and
metaTx.txRecord.params.executionSelector from calldata, enabling signer mismatch
attacks; fix by first comparing the signed handler fields to the actual handler
and selector the contract will use (require metaTx.params.handlerSelector ==
address(this).selector-equivalent or the actual entrypoint selector and
metaTx.params.handlerContract == address(this) as appropriate), and for flows
that operate on an existing txId (txCancellationWithMetaTx, approve/cancel
flows) require that metaTx.txRecord.params.executionSelector exactly matches
self.txRecords[txId].params.executionSelector (or rebuild the signed hash from
storage and validate equality) before calling
_validateExecutionAndHandlerPermissions or verifySignature; update
verifySignature/_validateExecutionAndHandlerPermissions call sites to perform
these equality checks first so signature and RBAC checks are applied to the
actual stored target, not attacker-supplied calldata.
In `@scripts/sanity-sdk/guard-controller/erc20-mint-controller-tests.ts`:
- Around line 66-82: The test suite never invokes
step2aMintMetaTxWrongActionMustRevert() from executeTests(), and the helper
currently treats any revert as success; update executeTests() to call
step2aMintMetaTxWrongActionMustRevert() in the correct sequence (after
step2Mint100ToAccountBloxViaMetaTx or where the wrong-action flow belongs) and
modify step2aMintMetaTxWrongActionMustRevert() to assert that the transaction
reverts with the specific decoded error name "NotSupported" (decode the revert
payload and assert equality rather than accepting any thrown error) so the test
fails on regressions to the NotSupported guard.
- Around line 901-948: The toHex helper inside normalizeMetaTxToHex can return
non-string values and only logs a warning; change it to validate inputs and
throw on unexpected types so it always returns a proper hex string.
Specifically, update normalizeMetaTxToHex -> toHex to: accept string/Uint8Array
(and strings starting with 0x), convert Uint8Array via bytesToHex, verify the
result matches /^0x[0-9a-fA-F]*$/ (or pad/normalize as needed), and throw an
Error for any other type or malformed value instead of returning '0x' or
logging; ensure callers (message, signature, data, txRecord.message,
params.executionSelector, params.operationType) rely on this invariant and
adjust any fallback logic to avoid silent non-string returns.
In `@scripts/sanity-sdk/runtime-rbac/rbac-tests.ts`:
- Around line 1108-1170: The helper ensureRoleHasRequiredPermissions currently
treats HandlerForSelectorMismatch as an acceptable outcome even when only one or
neither permission is verified; change it to require both verifyHandler and
verifyExecution to be true before proceeding. After the retry loop, remove the
early-success branches that allow partial success for
isHandlerMismatch/isResourceAlreadyExists; instead: if verifyHandler &&
verifyExecution, continue normally; else if isResourceAlreadyExists or
isHandlerMismatch but verification failed, throw a clear Error from
ensureRoleHasRequiredPermissions (include txStatus.error and roleHash) so the
setup gate fails fast; keep the special-case logging when verify succeeds to
allow re-runs, but do not let HandlerForSelectorMismatch turn this into
best-effort mode. Ensure references: isHandlerMismatch, isResourceAlreadyExists,
verifyHandler, verifyExecution, ROLE_CONFIG_BATCH_META_SELECTOR,
ROLE_CONFIG_BATCH_EXECUTE_SELECTOR, TxAction, and function
ensureRoleHasRequiredPermissions.
In `@scripts/sanity/guard-controller/erc20-mint-controller-tests.cjs`:
- Around line 96-101: The current idempotency branch checks only that roles
exist (using roleExists/getRoleHash and verifyStep1RolesAfter) but not that the
role members match expected wallets, so Step 1 can falsely pass; update the
branch in the handler that currently calls verifyStep1RolesAfter() and
passTest('Create MINT_REQUESTOR and MINT_APPROVER roles', ...) to also fetch and
compare the current role members to the expected wallet addresses (use whatever
helper exists for reading role members or add one), and only treat the step as
satisfied if both roles exist and their members exactly match the expected set;
if membership differs, proceed with the creation flow (or fail) instead of
passing. Ensure the same membership check is added to the other similar branches
around the lines that call verifyStep1RolesAfter() (the ones referenced near
lines 137-140 and 148-152).
- Around line 545-555: The test currently uses SIGN_META_REQUEST_AND_APPROVE and
calls requestAndApproveExecution, which exercises the legacy one-step handler;
change it to create a request-only meta-tx (use SIGN_META_REQUEST or the
equivalent constant in tests when calling createExternalExecutionMetaTx) and
then call the requestExecution flow instead of requestAndApproveExecution so the
code hits the timelock path; after requesting, simulate the approver's timelock
approval by invoking approveTimeLockExecutionWithMetaTx (or building an approve
meta-tx signed by mintApproverWallet and calling executeWithTimeLock) so the
test exercises executeWithTimeLock and approveTimeLockExecutionWithMetaTx rather
than the one-step handler.
- Around line 256-314: The final success gate is missing checks for the
handler-side SIGN_META_REQUEST_AND_APPROVE and several broadcaster permissions;
add roleHasPermissionForSelector calls to assert: (1) the handler/approver role
(handlerRoleHash or approverRoleHash as appropriate) has
SIGN_META_REQUEST_AND_APPROVE on REQUEST_AND_APPROVE_EXECUTION_SELECTOR, (2) the
broadcasterRoleHash has REQUEST_AND_APPROVE_EXECUTION_SELECTOR (with the correct
TxAction), and (3) the broadcasterRoleHash has
APPROVE_TIMELOCK_EXECUTION_META_SELECTOR and
CANCEL_TIMELOCK_EXECUTION_META_SELECTOR with the appropriate timelock TxAction;
store each result in a descriptive const (e.g.
handlerHasSignMetaRequestAndApprove, broadcasterHasHandlerSelector,
broadcasterHasApproveMetaTx, broadcasterHasCancelMetaTx) and include those
consts in the big if(...) alongside the existing requestorOk/.../broadcasterOk
checks so the idempotent gate only reports success when all permissions are
present (use roleHasPermissionForSelector to perform each check).
In `@test/foundry/helpers/PaymentTestHelper.sol`:
- Around line 185-198: The approveTransaction registration is enforcing handler
relations but approveTxHandlers only contains approveTxSelector, so handlers
won't match requests that advertise nativeTransferSelector; update the register
call in the block that builds approveTxHandlers used by
EngineBlox.registerFunction: either expand approveTxHandlers to length 2 and
push nativeTransferSelector alongside approveTxSelector (ensure you reference
nativeTransferSelector and approveTxHandlers) or set enforceHandlerRelations to
false in the EngineBlox.registerFunction call; make the minimal change
consistent with the helper's intended behavior (prefer adding
nativeTransferSelector if approvals should cover native transfers).
---
Nitpick comments:
In `@scripts/sanity-sdk/guard-controller/base-test.ts`:
- Around line 199-202: The revert logging can throw if decoded.params contains
bigint values because JSON.stringify fails on bigints; update the code that logs
and concatenates decoded.params (the use of decoded.params in the block using
extractErrorInfo and the console.error/ hint assembly) to serialize params with
a safe replacer that converts bigint (and other non-serializable numeric types)
to strings before JSON.stringify. Specifically, change the serialization of
decoded.params in the console.error and when building hint so it uses a
JSON.stringify call with a replacer that returns value.toString() for typeof
value === 'bigint' (and otherwise returns the value), ensuring no uncaught
TypeError from JSON.stringify.
In `@scripts/sanity/guard-controller/erc20-mint-controller-tests.cjs`:
- Around line 341-387: The test currently grants both EXECUTE_META_APPROVE and
EXECUTE_META_CANCEL (broadcasterApproveCancelActions) to both
APPROVE_TIMELOCK_EXECUTION_META_SELECTOR and
CANCEL_TIMELOCK_EXECUTION_META_SELECTOR, which is too broad; update the calls
that use addFunctionToRole for this.getRoleHash('BROADCASTER_ROLE') so
APPROVE_TIMELOCK_EXECUTION_META_SELECTOR receives only EXECUTE_META_APPROVE and
CANCEL_TIMELOCK_EXECUTION_META_SELECTOR receives only EXECUTE_META_CANCEL
(create two distinct action arrays or inline single-action arrays instead of
broadcasterApproveCancelActions), keeping the other
addFunctionToRoleWithHandlerForSelectors/addFunctionToRole calls and role hashes
(getRoleHash('BROADCASTER_ROLE'), REQUEST_AND_APPROVE_EXECUTION_SELECTOR,
ERC20_MINT_SELECTOR) unchanged.
In `@sdk/typescript/abi/IDefinition.abi.json`:
- Around line 33-41: The insertion of the new tuple field
enforceHandlerRelations before isProtected in the ABI shifts positional indexes
for FunctionSchema fields after supportedActionsBitmap; regenerate all generated
TypeScript bindings that consume IDefinition.abi (so FunctionSchema-related
types/parsers), update any consumers that destructure these tuples positionally
to use named accessors or the new index order (referencing
enforceHandlerRelations, isProtected, supportedActionsBitmap, FunctionSchema),
and publish this as a breaking SDK release with an appropriate major version
bump so downstream users are aware of the change.
In `@test/foundry/fuzz/ComprehensiveDefinitionSecurityFuzz.t.sol`:
- Around line 355-388: The fuzz input handlerSelector is never
used—testFuzz_DefinitionWithSelfReferenceBehavior always exercises the same
constant fixture (MaliciousDefinitions_InvalidSelfReference.HANDLER_SELECTOR);
update the test to use the fuzzed handlerSelector by either (A) passing
handlerSelector into the malicious fixture helpers (replace calls to
MaliciousDefinitions_InvalidSelfReference.getRolePermissions() and
.getFunctionSchemas() with variants that accept handlerSelector) or (B) build
the EngineBlox.FunctionSchema[] and IDefinition.RolePermission structs inline
using handlerSelector instead of
MaliciousDefinitions_InvalidSelfReference.HANDLER_SELECTOR before calling
testStateMachine.loadDefinitionsForTesting, so the fuzz input actually affects
the schema/permission and triggers different validation paths in
testFuzz_DefinitionWithSelfReferenceBehavior.
In `@test/foundry/fuzz/UnregisterFunctionFuzz.t.sol`:
- Around line 39-47: The harness currently calls EngineBlox.registerFunction
with enforceHandlerRelations hardcoded to true; change the fuzz input to include
a boolean for enforceHandlerRelations (fuzz both true and false) and add at
least one branch that constructs a mismatched handlers vs selectors scenario to
exercise the disabled-path edge cases (e.g., provide a handlers array that does
not align with functionSelector/bitmap) so both validation paths run; update the
call site in UnregisterFunctionFuzz.t.sol where
EngineBlox.registerFunction(state, functionSignature, functionSelector,
"TEST_OPERATION", bitmap, true, false, handlers) is invoked to use the new
fuzzed boolean and include a seeded mismatched-case path for handlers when the
flag is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7f86ee9e-df70-41ab-a711-8f55421a084d
📒 Files selected for processing (56)
abi/AccountBlox.abi.jsonabi/BaseStateMachine.abi.jsonabi/CopyBlox.abi.jsonabi/GuardController.abi.jsonabi/GuardControllerDefinitions.abi.jsonabi/IDefinition.abi.jsonabi/RuntimeRBAC.abi.jsonabi/RuntimeRBACDefinitions.abi.jsonabi/SecureOwnable.abi.jsonabi/SecureOwnableDefinitions.abi.jsonabi/SimpleRWA20.abi.jsonabi/SimpleRWA20Definitions.abi.jsonabi/SimpleVault.abi.jsonabi/SimpleVaultDefinitions.abi.jsoncontracts/core/access/lib/definitions/RuntimeRBACDefinitions.solcontracts/core/base/BaseStateMachine.solcontracts/core/execution/GuardController.solcontracts/core/execution/lib/definitions/GuardControllerDefinitions.solcontracts/core/lib/EngineBlox.solcontracts/core/security/lib/definitions/SecureOwnableDefinitions.solcontracts/examples/applications/PayBlox/PayBloxDefinitions.solcontracts/examples/applications/SimpleRWA20/SimpleRWA20Definitions.solcontracts/examples/applications/SimpleVault/SimpleVaultDefinitions.solcontracts/examples/integrations/Safe/GuardianSafe/GuardianSafeDefinitions.solpackage/package.jsonscripts/sanity-sdk/guard-controller/base-test.tsscripts/sanity-sdk/guard-controller/erc20-mint-controller-tests.tsscripts/sanity-sdk/run-all-tests.tsscripts/sanity-sdk/runtime-rbac/base-test.tsscripts/sanity-sdk/runtime-rbac/rbac-tests.tsscripts/sanity-sdk/secure-ownable/base-test.tsscripts/sanity/guard-controller/base-test.cjsscripts/sanity/guard-controller/erc20-mint-controller-tests.cjssdk/typescript/abi/AccountBlox.abi.jsonsdk/typescript/abi/BaseStateMachine.abi.jsonsdk/typescript/abi/CopyBlox.abi.jsonsdk/typescript/abi/GuardController.abi.jsonsdk/typescript/abi/GuardControllerDefinitions.abi.jsonsdk/typescript/abi/IDefinition.abi.jsonsdk/typescript/abi/RuntimeRBAC.abi.jsonsdk/typescript/abi/RuntimeRBACDefinitions.abi.jsonsdk/typescript/abi/SecureOwnable.abi.jsonsdk/typescript/abi/SecureOwnableDefinitions.abi.jsonsdk/typescript/abi/SimpleRWA20.abi.jsonsdk/typescript/abi/SimpleRWA20Definitions.abi.jsonsdk/typescript/abi/SimpleVault.abi.jsonsdk/typescript/abi/SimpleVaultDefinitions.abi.jsonsdk/typescript/contracts/core/BaseStateMachine.tsxsdk/typescript/interfaces/base.index.tsxsdk/typescript/package.jsonsdk/typescript/types/definition.index.tsxtest/foundry/fuzz/ComprehensiveDefinitionSecurityFuzz.t.soltest/foundry/fuzz/UnregisterFunctionFuzz.t.soltest/foundry/helpers/MaliciousDefinitions.soltest/foundry/helpers/PaymentTestHelper.soltest/foundry/helpers/TestDefinitionContracts.sol
Comment on lines
+923
to
+927
| { | ||
| "internalType": "bool", | ||
| "name": "enforceHandlerRelations", | ||
| "type": "bool" | ||
| }, |
Contributor
There was a problem hiding this comment.
Avoid inserting the new field mid-tuple unless this is a breaking release.
Adding enforceHandlerRelations before isProtected changes the positional ABI layout of FunctionSchema. Any client decoding this tuple positionally will now read the old isProtected slot as the new boolean and shift the remaining fields. If backward compatibility matters, append the field at the end instead, or ship this behind an explicit breaking-version/migration update.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@abi/CopyBlox.abi.json` around lines 923 - 927, You inserted the new boolean
enforceHandlerRelations into the middle of the FunctionSchema tuple (before
isProtected), which changes positional ABI layout and will break positional
decoders; to fix, move the enforceHandlerRelations field to the end of the
FunctionSchema tuple (or alternatively only introduce it as part of an explicit
breaking-version/migration), updating the ABI so the original order of existing
fields (including isProtected) is preserved; reference the FunctionSchema tuple
and the fields enforceHandlerRelations and isProtected when making the change.
Comment on lines
+163
to
+167
| { | ||
| "internalType": "bool", | ||
| "name": "enforceHandlerRelations", | ||
| "type": "bool" | ||
| }, |
Contributor
There was a problem hiding this comment.
The new schema flag is not configurable through this helper ABI.
getFunctionSchemas now exposes enforceHandlerRelations, but this definitions surface still only exposes encodeRegisterFunction(string,string,TxAction[]). Callers using the helper API have no way to set the new flag when producing registration payloads, so they either get stale behavior or an implicit hardcoded default. Please thread the boolean through the register-function helper and regenerate the ABI/SDK artifacts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@abi/GuardControllerDefinitions.abi.json` around lines 163 - 167, The helper
ABI must accept and pass the new enforceHandlerRelations flag: update the helper
signature for encodeRegisterFunction (currently
encodeRegisterFunction(string,string,TxAction[])) to include a boolean parameter
for enforceHandlerRelations, thread that boolean into the payload construction
where getFunctionSchemas/enforceHandlerRelations is set, and regenerate the ABI
and SDK artifacts so callers can supply the flag; ensure all usages of
encodeRegisterFunction, any wrapper helpers, and tests are updated to pass the
new boolean.
Comment on lines
+1045
to
+1049
| { | ||
| "internalType": "bool", | ||
| "name": "enforceHandlerRelations", | ||
| "type": "bool" | ||
| }, |
Contributor
There was a problem hiding this comment.
This is a breaking ABI reorder.
Inserting enforceHandlerRelations before isProtected shifts the positional tuple layout of EngineBlox.FunctionSchema. Any existing consumer decoding getFunctionSchema() by index will now read the wrong slot, so this needs to be treated as an explicit breaking change or appended at the end of the struct instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@abi/SimpleVault.abi.json` around lines 1045 - 1049, The ABI change inserted
the new boolean field enforceHandlerRelations before isProtected, which alters
the positional tuple layout of EngineBlox.FunctionSchema and will break any
consumer decoding getFunctionSchema() by index; restore compatibility by moving
enforceHandlerRelations to the end of the struct/ABI ordering (or explicitly
mark this as a breaking change and bump the contract/ABI version), update any
consumers to use named decoding instead of index-based access, and ensure the
final ABI keeps the original field order (isProtected in its original position)
unless you deliberately release a breaking version.
Comment on lines
+97
to
98
| enforceHandlerRelations: true, | ||
| isProtected: true, |
Contributor
There was a problem hiding this comment.
cancelTransaction is not fully wired for enforced handler relations.
With enforceHandlerRelations enabled, the cancel path now relies on the linked EXEC_SAFE_TX_SELECTOR, but executionActions and the owner's EXEC_SAFE_TX_SELECTOR permission set still omit EngineBlox.TxAction.EXECUTE_TIME_DELAY_CANCEL. That leaves cancelTransaction(uint256) authorized at the handler layer and rejected at the linked execution-selector layer.
🐛 Suggested fix
- EngineBlox.TxAction[] memory executionActions = new EngineBlox.TxAction[](8);
+ EngineBlox.TxAction[] memory executionActions = new EngineBlox.TxAction[](9);
executionActions[0] = EngineBlox.TxAction.EXECUTE_TIME_DELAY_REQUEST;
executionActions[1] = EngineBlox.TxAction.EXECUTE_TIME_DELAY_APPROVE;
+ executionActions[2] = EngineBlox.TxAction.EXECUTE_TIME_DELAY_CANCEL;
- executionActions[2] = EngineBlox.TxAction.SIGN_META_APPROVE;
- executionActions[3] = EngineBlox.TxAction.SIGN_META_CANCEL;
- executionActions[4] = EngineBlox.TxAction.SIGN_META_REQUEST_AND_APPROVE;
- executionActions[5] = EngineBlox.TxAction.EXECUTE_META_APPROVE;
- executionActions[6] = EngineBlox.TxAction.EXECUTE_META_CANCEL;
- executionActions[7] = EngineBlox.TxAction.EXECUTE_META_REQUEST_AND_APPROVE;
+ executionActions[3] = EngineBlox.TxAction.SIGN_META_APPROVE;
+ executionActions[4] = EngineBlox.TxAction.SIGN_META_CANCEL;
+ executionActions[5] = EngineBlox.TxAction.SIGN_META_REQUEST_AND_APPROVE;
+ executionActions[6] = EngineBlox.TxAction.EXECUTE_META_APPROVE;
+ executionActions[7] = EngineBlox.TxAction.EXECUTE_META_CANCEL;
+ executionActions[8] = EngineBlox.TxAction.EXECUTE_META_REQUEST_AND_APPROVE;
- EngineBlox.TxAction[] memory ownerExecutionActions = new EngineBlox.TxAction[](5);
+ EngineBlox.TxAction[] memory ownerExecutionActions = new EngineBlox.TxAction[](6);
ownerExecutionActions[0] = EngineBlox.TxAction.EXECUTE_TIME_DELAY_REQUEST;
ownerExecutionActions[1] = EngineBlox.TxAction.EXECUTE_TIME_DELAY_APPROVE;
- ownerExecutionActions[2] = EngineBlox.TxAction.SIGN_META_APPROVE;
- ownerExecutionActions[3] = EngineBlox.TxAction.SIGN_META_CANCEL;
- ownerExecutionActions[4] = EngineBlox.TxAction.SIGN_META_REQUEST_AND_APPROVE;
+ ownerExecutionActions[2] = EngineBlox.TxAction.EXECUTE_TIME_DELAY_CANCEL;
+ ownerExecutionActions[3] = EngineBlox.TxAction.SIGN_META_APPROVE;
+ ownerExecutionActions[4] = EngineBlox.TxAction.SIGN_META_CANCEL;
+ ownerExecutionActions[5] = EngineBlox.TxAction.SIGN_META_REQUEST_AND_APPROVE;As per coding guidelines, "Implement role-based access control (RBAC) with per-function permissions and protected roles."
Also applies to: 157-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@contracts/examples/integrations/Safe/GuardianSafe/GuardianSafeDefinitions.sol`
around lines 97 - 98, The cancelTransaction(uint256) path is not authorized in
the execution-selector layer when enforceHandlerRelations is true; update the
permissions so the handler and the linked EXEC_SAFE_TX_SELECTOR agree: add
EngineBlox.TxAction.EXECUTE_TIME_DELAY_CANCEL to executionActions and to the
owner's EXEC_SAFE_TX_SELECTOR permission set (and any other owner/handler
permission mappings touched by enforceHandlerRelations) so cancelTransaction is
authorized end-to-end when enforceHandlerRelations and isProtected are enabled.
Comment on lines
+171
to
+188
| const allPassed = this.results.failed === 0; | ||
|
|
||
| if (allPassed) { | ||
| console.log('\n🎉 All tests passed!'); | ||
| process.exit(0); | ||
| } else { | ||
| console.log('\n⚠️ Some tests failed. Please review the output above.'); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| // Post-sanity system state summary (only when all tests passed) | ||
| try { | ||
| await this.printPostSanitySystemStateSummary(testsToRun); | ||
| } catch (e: any) { | ||
| console.warn( | ||
| `\n⚠️ Post-sanity system state summary failed: ${e?.message ?? String(e)}` | ||
| ); | ||
| } | ||
|
|
||
| process.exit(allPassed ? 0 : 1); |
Contributor
There was a problem hiding this comment.
The post-sanity state dump still runs after failures.
The comment says this should run only on an all-green pass, but the call is unconditional. Failed runs will still print chain state, which is usually stale or partially-mutated and makes the failure summary noisier.
💡 Suggested fix
- // Post-sanity system state summary (only when all tests passed)
- try {
- await this.printPostSanitySystemStateSummary(testsToRun);
- } catch (e: any) {
- console.warn(
- `\n⚠️ Post-sanity system state summary failed: ${e?.message ?? String(e)}`
- );
+ if (allPassed) {
+ try {
+ await this.printPostSanitySystemStateSummary(testsToRun);
+ } catch (e: any) {
+ console.warn(
+ `\n⚠️ Post-sanity system state summary failed: ${e?.message ?? String(e)}`
+ );
+ }
}
process.exit(allPassed ? 0 : 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sanity-sdk/run-all-tests.ts` around lines 171 - 188, The post-sanity
system state summary is being executed unconditionally; guard the call to
printPostSanitySystemStateSummary so it only runs when allPassed is true (i.e.,
the all-green case). Move the try/catch that calls
this.printPostSanitySystemStateSummary(testsToRun) inside the if (allPassed)
branch (or wrap it with if (allPassed) { ... }) and keep the existing
process.exit(allPassed ? 0 : 1) behavior unchanged so failures do not trigger
the post-sanity dump.
Comment on lines
+1144
to
+1148
| { | ||
| "internalType": "bool", | ||
| "name": "enforceHandlerRelations", | ||
| "type": "bool" | ||
| }, |
Contributor
There was a problem hiding this comment.
This is a breaking ABI reorder.
Adding the new field in the middle of EngineBlox.FunctionSchema changes the tuple indexes returned by getFunctionSchema(). Any SDK consumer relying on positional decoding now gets a different layout, so this should either be appended at the end of the struct or released as an explicitly breaking ABI change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/typescript/abi/BaseStateMachine.abi.json` around lines 1144 - 1148, The
ABI change inserted the new boolean field "enforceHandlerRelations" into the
middle of EngineBlox.FunctionSchema, which shifts tuple indexes returned by
getFunctionSchema(); to fix this, move the "enforceHandlerRelations" entry to
the end of the EngineBlox.FunctionSchema struct in BaseStateMachine.abi.json so
existing positional decoding remains stable, or if intentional, mark and release
the ABI as a breaking change and document that getFunctionSchema() tuple indexes
have changed.
Comment on lines
+861
to
+865
| { | ||
| "internalType": "bool", | ||
| "name": "enforceHandlerRelations", | ||
| "type": "bool" | ||
| }, |
Contributor
There was a problem hiding this comment.
This is a breaking ABI reorder.
EngineBlox.FunctionSchema is a public tuple shape here, and inserting enforceHandlerRelations before isProtected shifts every later index. That breaks existing positional decoders, so this should be treated as a breaking release or moved to the end of the struct layout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/typescript/abi/RuntimeRBAC.abi.json` around lines 861 - 865, The new bool
field enforceHandlerRelations was inserted before isProtected in the public
tuple EngineBlox.FunctionSchema, shifting all subsequent tuple indices and
breaking positional decoders; fix by moving enforceHandlerRelations to the end
of the FunctionSchema tuple layout (append after all existing fields) so indices
for existing fields remain stable, or if you must keep the new position, treat
it as a breaking ABI change by bumping the ABI/contract version and update any
positional decoders, serializers/deserializers and tests that reference
EngineBlox.FunctionSchema or the isProtected index accordingly.
Comment on lines
+72
to
+76
| { | ||
| "internalType": "bool", | ||
| "name": "enforceHandlerRelations", | ||
| "type": "bool" | ||
| }, |
Contributor
There was a problem hiding this comment.
This is a breaking ABI reorder.
This inserts enforceHandlerRelations into the middle of the returned EngineBlox.FunctionSchema tuple, which shifts the index of isProtected and everything after it. If compatibility matters for current SDK consumers, append the field at the end of the struct and regenerate the ABI instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/typescript/abi/RuntimeRBACDefinitions.abi.json` around lines 72 - 76, The
ABI change inserted "enforceHandlerRelations" into the middle of
EngineBlox.FunctionSchema and shifted subsequent tuple indexes (including
"isProtected"), which breaks compatibility; move "enforceHandlerRelations" to
the end of the FunctionSchema tuple instead of inserting it in the middle and
then regenerate the ABI (RuntimeRBACDefinitions.abi.json) so the tuple order
preserves existing indexes and consumers remain compatible.
Comment on lines
+111
to
+115
| { | ||
| "internalType": "bool", | ||
| "name": "enforceHandlerRelations", | ||
| "type": "bool" | ||
| }, |
Contributor
There was a problem hiding this comment.
This is a breaking ABI reorder.
Because getFunctionSchemas() returns an array of EngineBlox.FunctionSchema, inserting the new boolean before isProtected changes the positional layout of every element. Existing decoders will mis-map fields unless this is treated as an explicit breaking change or the field is appended instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/typescript/abi/SimpleRWA20Definitions.abi.json` around lines 111 - 115,
The ABI insertion of "enforceHandlerRelations" before "isProtected" breaks
positional decoding for getFunctionSchemas() which returns an array of
EngineBlox.FunctionSchema; either move the "enforceHandlerRelations" entry to
the end of the function schema object array (append it after existing fields) to
preserve existing field positions, or treat this as an explicit breaking change
by bumping the ABI/schema version and documenting the reorder; locate the
"enforceHandlerRelations" field in SimpleRWA20Definitions.abi.json and adjust
its position (or version) so decoders relying on EngineBlox.FunctionSchema
maintain correct positional mappings.
Comment on lines
+1045
to
+1049
| { | ||
| "internalType": "bool", | ||
| "name": "enforceHandlerRelations", | ||
| "type": "bool" | ||
| }, |
Contributor
There was a problem hiding this comment.
This is a breaking ABI reorder.
The new field is inserted before isProtected, so the positional return layout of EngineBlox.FunctionSchema changes in the SDK ABI as well. Any existing client reading tuple members by index will deserialize the wrong values unless this is shipped as an explicit breaking change or the field is appended instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/typescript/abi/SimpleVault.abi.json` around lines 1045 - 1049, The ABI
change inserted the new boolean field enforceHandlerRelations before
isProtected, altering the positional tuple layout used by
EngineBlox.FunctionSchema and breaking clients that index tuple members; to fix,
move enforceHandlerRelations to the end of the return tuple (append it after
existing fields) in SimpleVault.abi.json so existing positional deserialization
remains stable, or if deliberate, mark this as an explicit breaking ABI change
in the SDK release notes and bump the ABI version referenced by
EngineBlox.FunctionSchema.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores