Conversation
This commit improves the role management functionality in the EngineBlox library by ensuring a thorough cleanup of associated data when a role is removed. The changes include clearing the `authorizedWallets` set and removing function permissions and selectors to prevent stale data during role recreation. This enhancement ensures better integrity and efficiency in role-based access control, contributing to a more robust implementation. Additionally, comments have been updated for clarity on the cleanup process.
…esses This commit introduces a new internal function, `_validateMetaTxMatchRecord`, to ensure that the meta-transaction's record matches the stored transaction record for a given transaction ID. This validation prevents unauthorized modifications during the approval and cancellation flows by checking critical fields such as execution selector, target, value, requester, gas limit, operation type, and release time. Additionally, a new error, `MetaTxRecordMismatchStoredTx`, is defined to handle mismatches effectively. These enhancements improve the integrity and security of the meta-transaction handling in the EngineBlox library.
…ineBlox library This commit adds a custom security annotation to the `_txApprovalWithMetaTx` function, clarifying that the releaseTime (timelock) is intentionally not enforced on this path. The documentation explains the design choice to allow authorized signers to approve execution without waiting for the releaseTime, highlighting the hybrid synergy between timelock workflows and meta-transaction workflows. This enhancement improves understanding of the function's behavior and security implications for developers interacting with the EngineBlox library.
…gineBlox library This commit introduces a new internal function, `_validateMetaTxPaymentMatchRecord`, to ensure that the payment details of a meta-transaction match the stored transaction record for a given transaction ID. This validation checks critical fields such as recipient, native token amount, ERC20 token address, and ERC20 token amount, preventing unauthorized modifications to payment details. Additionally, a new error, `MetaTxPaymentMismatchStoredTx`, is defined to handle mismatches effectively. These enhancements improve the integrity and security of the meta-transaction handling in the EngineBlox library.
…lox library This commit enhances the message hashing process for meta-transactions by introducing a selective approach that focuses on the essential components: MetaTxRecord, TxParams, and PaymentDetails. The new structure improves the integrity of the generated message hash, ensuring that only relevant data is included for signing. Additionally, the documentation for the `generateMessageHash` and `recoverSigner` functions has been updated to reflect these changes, clarifying the signing process and the expected structure for clients. These improvements contribute to a more secure and efficient handling of meta-transactions within the EngineBlox library.
…role policies This commit improves the documentation in the RuntimeRBAC and EngineBlox libraries by detailing the security policies surrounding protected roles. Key updates include clarifications on the unauthorized modification of protected roles, the necessity of role configuration batch ordering, and the enforcement of protected-role checks within the EngineBlox library. Additionally, custom security annotations have been added to emphasize the layered defense strategy and the exclusive authority of SecureOwnable in managing system wallets. These enhancements aim to provide clearer guidance for developers and reinforce the security framework of the role-based access control system.
…Blox library This commit introduces a call to the new internal function `_validateMetaTxPaymentMatchRecord` within the meta-transaction execution flow. This enhancement ensures that the payment details of a meta-transaction are validated against the stored transaction record, reinforcing the integrity of the payment process. By implementing this check, the library improves its security posture against unauthorized payment modifications during execution. These changes contribute to a more robust handling of meta-transactions in the EngineBlox library.
…Blox library This commit updates the documentation for the EIP-712 message hashing and signature recovery processes within the EngineBlox library. It emphasizes that integrators must sign the contract-provided digest directly using `personal_sign(metaTx.message)` instead of generic `eth_signTypedData_v4` serializers, as the contract constructs a custom domain separator and type hash. These changes ensure that the signing process is clear and that clients understand the implications of using standard serializers, which may produce incompatible hashes. Additionally, the validation function for inclusive ranges in the SharedValidation documentation has been updated for clarity.
…gineBlox library This commit updates the documentation for the EIP-712 message hashing and signature recovery processes within the EngineBlox library. It clarifies that integrators must sign the digest as a raw hash without any EIP-191 or personal_sign prefix, ensuring compatibility with the contract's verification method. The changes emphasize the importance of using the correct signing approach to avoid incompatible hashes, and the resulting digest is now included in the `message` field of the `MetaTransaction` structs for easier access. These enhancements aim to improve the clarity and security of the signing process for integrators.
This commit introduces a new validation mechanism within the EngineBlox library to ensure that the execution selector is part of the handler selector's schema flow in strict mode. The addition of the `_schemaHasHandlerSelector` function allows for a more robust check against unauthorized handler executions, enhancing the security of the meta-transaction processing. This change aims to prevent mismatches between execution and handler selectors, thereby reinforcing the integrity of the function schema relationships. Overall, these enhancements contribute to a more secure and reliable execution environment for meta-transactions.
This commit updates the PaymentTestHelper contract to include an additional execution selector in the handler selectors for the approveTransaction function. The new structure now accommodates both the self-reference and the allowed execution selector, improving the flexibility and security of the transaction approval process. This enhancement ensures that the contract can effectively manage the execution flow of approved transactions, contributing to a more robust meta-transaction handling mechanism.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR transitions from EIP-191 (Ethereum-prefixed) message signing to raw EIP-712 digest signing throughout the stack. It introduces new meta-transaction validation errors, refactors monolithic EIP-712 type hashes into granular typed constants, adds meta-tx record and payment matching validators, and enhances handler-to-execution selector enforcement with optional strictness control. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/foundry/fuzz/ComprehensiveStateMachineFuzz.t.sol (1)
997-1000:⚠️ Potential issue | 🟡 MinorAssertion is always true due to
|| true.The condition
result.result.length > 0 || truealways evaluates totrue, making this assertion meaningless. If you intend to verify that the failure was recorded with a reason, remove|| true. If this check is intentionally optional, consider removing the assertion or converting it to a comment.🔧 Proposed fix
if (result.status == EngineBlox.TxStatus.FAILED) { // EIP-150: low gas caused failure; catch path must not have set COMPLETED - assertTrue(result.result.length > 0 || true, "Failure recorded"); + // Note: result.result may be empty for OOG failures }Or, if the check is meaningful:
if (result.status == EngineBlox.TxStatus.FAILED) { // EIP-150: low gas caused failure; catch path must not have set COMPLETED - assertTrue(result.result.length > 0 || true, "Failure recorded"); + assertTrue(result.result.length > 0, "Failure recorded"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/foundry/fuzz/ComprehensiveStateMachineFuzz.t.sol` around lines 997 - 1000, The assertion in the failure branch uses a tautology ("result.result.length > 0 || true") so it never checks anything; update the check inside the block that handles EngineBlox.TxStatus.FAILED (where `result` is inspected) by removing the `|| true` to assert the intended condition (`result.result.length > 0`) or, if you meant the check to be optional, delete the assertion entirely or replace it with a descriptive comment explaining why no assertion is required.sdk/typescript/utils/metaTx/metaTransaction.tsx (1)
188-209:⚠️ Potential issue | 🟠 MajorCheck the recovered signer against
unsignedMetaTx.params.signer.This currently verifies the signature against the extra
signerAddressargument only, so the SDK can return a “signed” meta-tx that the contract will reject wheneversignerAddress/privateKeyandunsignedMetaTx.params.signerdiverge.💡 Suggested fix
async signMetaTransaction( unsignedMetaTx: MetaTransaction, signerAddress: Address, privateKey: Hex ): Promise<MetaTransaction> { @@ const { recoverAddress } = await import('viem'); const recoveredAddress = await recoverAddress({ hash: contractDigest, signature }); - if (recoveredAddress.toLowerCase() !== signerAddress.toLowerCase()) { + const expectedSigner = unsignedMetaTx.params.signer; + if ( + signerAddress.toLowerCase() !== expectedSigner.toLowerCase() || + recoveredAddress.toLowerCase() !== expectedSigner.toLowerCase() + ) { throw new Error('Signature verification failed'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/typescript/utils/metaTx/metaTransaction.tsx` around lines 188 - 209, In signMetaTransaction, the signature is being verified against the passed-in signerAddress instead of the meta-tx's declared signer; update the verification to compare recoveredAddress (from recoverAddress) to unsignedMetaTx.params.signer (or ensure both match), and throw the Error if they differ; locate the check in signMetaTransaction and replace or extend the comparison that currently references signerAddress with unsignedMetaTx.params.signer to ensure the signed payload matches the contract-expected signer.
🧹 Nitpick comments (2)
test/foundry/fuzz/ProtectedResourceFuzz.t.sol (1)
79-108: Consider expanding coverage to all protected roles like the ADD_WALLET test.The function is prefixed with
testFuzz_but has no fuzz parameters. Also, unliketestFuzz_CannotAddWalletToProtectedRolewhich tests all three protected roles, this only tests revoking owner from OWNER_ROLE.♻️ Suggested improvement for consistency
- function testFuzz_CannotRevokeWalletFromProtectedRole() public { - // Test revoking owner from OWNER_ROLE + function test_CannotRevokeWalletFromProtectedRole() public { + // Test revoking wallets from protected roles + bytes32[3] memory protectedRoles = [OWNER_ROLE, BROADCASTER_ROLE, RECOVERY_ROLE]; + address[3] memory protectedWallets = [owner, broadcaster, recovery]; + + for (uint256 i = 0; i < protectedRoles.length; i++) { IRuntimeRBAC.RoleConfigAction[] memory actions = new IRuntimeRBAC.RoleConfigAction[](1); actions[0] = IRuntimeRBAC.RoleConfigAction({ actionType: IRuntimeRBAC.RoleConfigActionType.REVOKE_WALLET, - data: abi.encode(OWNER_ROLE, owner) + data: abi.encode(protectedRoles[i], protectedWallets[i]) }); + // ... rest of test logic + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/foundry/fuzz/ProtectedResourceFuzz.t.sol` around lines 79 - 108, The test testFuzz_CannotRevokeWalletFromProtectedRole only checks OWNER_ROLE and is not actually fuzzing; change it to cover all protected roles like testFuzz_CannotAddWalletToProtectedRole by iterating or fuzzing over the protected role set (OWNER_ROLE, ADMIN_ROLE, OPERATOR_ROLE) and using that role variable when building actions (IRuntimeRBAC.RoleConfigAction data), executionParams, metaTx (_createMetaTxForRoleConfig), and the expectedError (SharedValidation.CannotModifyProtected.selector, role); ensure the test name/fuzzer signature matches (either add a role parameter to make it a true fuzz test or loop through the three roles inside the function) and update assertions to use the variable role instead of the hardcoded OWNER_ROLE.contracts/core/lib/utils/SharedValidation.sol (1)
410-415: Function namevalidateLessThanis now misleading after semantic change.The condition changed from
from >= totofrom > to, which means the function now validatesfrom <= to(inclusive range). The NatSpec is correctly updated, but the function name suggests strict less-than semantics.Consider renaming to
validateLessThanOrEqualorvalidateInclusiveRangeto match the new behavior and prevent confusion for future maintainers.♻️ Suggested rename for clarity
- function validateLessThan(uint256 from, uint256 to) internal pure { + function validateLessThanOrEqual(uint256 from, uint256 to) internal pure { if (from > to) revert InvalidRange(from, to); }Note: This would require updating the call site in
BaseStateMachine.getTransactionHistory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/core/lib/utils/SharedValidation.sol` around lines 410 - 415, The function validateLessThan now enforces an inclusive check (from <= to) but its name implies strict less-than; rename the function to validateLessThanOrEqual (or validateInclusiveRange) and update all call sites (notably BaseStateMachine.getTransactionHistory) to use the new name, preserving the logic (if (from > to) revert InvalidRange(from, to)) and keeping the NatSpec aligned with the new name to avoid confusion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/core/lib/EngineBlox.sol`:
- Around line 2262-2272: The strict-mode branch only checks the schema-level
relation but misses enforcing the role-level handler→execution pairing; update
the branch where handlerSchema.enforceHandlerRelations is checked so that after
validating _schemaHasHandlerSelector, also verify the active role's
FunctionPermission.handlerForSelectors for handlerSelector includes
executionSelector (i.e., ensure the role-specific bitmap allows that execution),
using the same role identifier/context that hasActionPermission() uses; if the
role-level mapping does not permit it, revert with
SharedValidation.HandlerForSelectorMismatch (same as current) — reference
FunctionSchema, handlerSchema.enforceHandlerRelations, hasActionPermission(),
FunctionPermission.handlerForSelectors, and _schemaHasHandlerSelector to locate
the code to change.
- Around line 153-157: The new bool enforceHandlerRelations was inserted before
isProtected in the FunctionSchema struct, shifting storage and causing old
packed isProtected bits to be misread; to fix, revert that insertion order and
preserve layout by either appending enforceHandlerRelations to the end of the
struct or by adding a reserved storage gap (e.g., uint256[] __gap sized to cover
the added slot) so existing fields (isProtected and handlerForSelectors) keep
their original positions; update the FunctionSchema definition (and any related
structs) to use the gap pattern or move enforceHandlerRelations to the struct
tail to avoid breaking persisted state.
In `@scripts/sanity/utils/eip712-signing.cjs`:
- Around line 186-189: The change to signMetaTransaction() returned shape broke
callers expecting signature.signature; revert or provide the previous nested
shape by returning the metaTx plus a signature object containing the hex and
message (e.g. keep signMetaTransaction(...) returning { ...metaTx, signature: {
signature: signatureHex, message: messageHash } }) so existing callers that read
signature.signature continue to work, or alternatively update those callers to
read signature (but prefer restoring the nested shape in signMetaTransaction to
maintain backward compatibility).
---
Outside diff comments:
In `@sdk/typescript/utils/metaTx/metaTransaction.tsx`:
- Around line 188-209: In signMetaTransaction, the signature is being verified
against the passed-in signerAddress instead of the meta-tx's declared signer;
update the verification to compare recoveredAddress (from recoverAddress) to
unsignedMetaTx.params.signer (or ensure both match), and throw the Error if they
differ; locate the check in signMetaTransaction and replace or extend the
comparison that currently references signerAddress with
unsignedMetaTx.params.signer to ensure the signed payload matches the
contract-expected signer.
In `@test/foundry/fuzz/ComprehensiveStateMachineFuzz.t.sol`:
- Around line 997-1000: The assertion in the failure branch uses a tautology
("result.result.length > 0 || true") so it never checks anything; update the
check inside the block that handles EngineBlox.TxStatus.FAILED (where `result`
is inspected) by removing the `|| true` to assert the intended condition
(`result.result.length > 0`) or, if you meant the check to be optional, delete
the assertion entirely or replace it with a descriptive comment explaining why
no assertion is required.
---
Nitpick comments:
In `@contracts/core/lib/utils/SharedValidation.sol`:
- Around line 410-415: The function validateLessThan now enforces an inclusive
check (from <= to) but its name implies strict less-than; rename the function to
validateLessThanOrEqual (or validateInclusiveRange) and update all call sites
(notably BaseStateMachine.getTransactionHistory) to use the new name, preserving
the logic (if (from > to) revert InvalidRange(from, to)) and keeping the NatSpec
aligned with the new name to avoid confusion.
In `@test/foundry/fuzz/ProtectedResourceFuzz.t.sol`:
- Around line 79-108: The test testFuzz_CannotRevokeWalletFromProtectedRole only
checks OWNER_ROLE and is not actually fuzzing; change it to cover all protected
roles like testFuzz_CannotAddWalletToProtectedRole by iterating or fuzzing over
the protected role set (OWNER_ROLE, ADMIN_ROLE, OPERATOR_ROLE) and using that
role variable when building actions (IRuntimeRBAC.RoleConfigAction data),
executionParams, metaTx (_createMetaTxForRoleConfig), and the expectedError
(SharedValidation.CannotModifyProtected.selector, role); ensure the test
name/fuzzer signature matches (either add a role parameter to make it a true
fuzz test or loop through the three roles inside the function) and update
assertions to use the variable role instead of the hardcoded OWNER_ROLE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 217f2c4b-ff71-4a1a-b939-65e16d6014b8
📒 Files selected for processing (24)
abi/EngineBlox.abi.jsoncontracts/core/access/RuntimeRBAC.solcontracts/core/lib/EngineBlox.solcontracts/core/lib/utils/SharedValidation.soldocs/core/lib/utils/SharedValidation.mdscripts/sanity/utils/eip712-signing.cjssdk/typescript/abi/EngineBlox.abi.jsonsdk/typescript/lib/EngineBlox.tsxsdk/typescript/utils/metaTx/metaTransaction.tsxtest/foundry/fuzz/ComprehensiveAccessControlFuzz.t.soltest/foundry/fuzz/ComprehensiveCompositeFuzz.t.soltest/foundry/fuzz/ComprehensiveGasExhaustionFuzz.t.soltest/foundry/fuzz/ComprehensiveInputValidationFuzz.t.soltest/foundry/fuzz/ComprehensiveMetaTransactionFuzz.t.soltest/foundry/fuzz/ComprehensiveSecurityEdgeCasesFuzz.t.soltest/foundry/fuzz/ComprehensiveStateMachineFuzz.t.soltest/foundry/fuzz/EdgeCasesFuzz.t.soltest/foundry/fuzz/MetaTransactionSecurityFuzz.t.soltest/foundry/fuzz/ProtectedResourceFuzz.t.soltest/foundry/fuzz/RBACPermissionFuzz.t.soltest/foundry/fuzz/RuntimeRBACFuzz.t.soltest/foundry/helpers/PaymentTestHelper.soltest/foundry/helpers/TestHelpers.soltest/foundry/invariant/StateMachineInvariants.t.sol
| /// @dev When true (strict mode): handlerForSelectors in role permissions must match this schema's handlerForSelectors at use time. | ||
| /// When false (flexible mode): no such check; forward references and unregistered selectors in handlerForSelectors are allowed at registration. | ||
| bool enforceHandlerRelations; | ||
| bool isProtected; | ||
| bytes4[] handlerForSelectors; | ||
| bytes4[] handlerForSelectors; |
There was a problem hiding this comment.
Appending enforceHandlerRelations here breaks persisted FunctionSchema state.
For any in-place upgrade, the old packed isProtected bit will now be read as enforceHandlerRelations, and isProtected will read back as false. Previously protected schemas would become mutable after upgrade.
💡 Safer layout
uint16 supportedActionsBitmap; // Bitmap for TxAction enum (9 bits max)
- bool enforceHandlerRelations;
bool isProtected;
bytes4[] handlerForSelectors;
+ bool enforceHandlerRelations;🤖 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 153 - 157, The new bool
enforceHandlerRelations was inserted before isProtected in the FunctionSchema
struct, shifting storage and causing old packed isProtected bits to be misread;
to fix, revert that insertion order and preserve layout by either appending
enforceHandlerRelations to the end of the struct or by adding a reserved storage
gap (e.g., uint256[] __gap sized to cover the added slot) so existing fields
(isProtected and handlerForSelectors) keep their original positions; update the
FunctionSchema definition (and any related structs) to use the gap pattern or
move enforceHandlerRelations to the struct tail to avoid breaking persisted
state.
| // In strict mode, enforce that the executionSelector is part of the handlerSelector's schema flow. | ||
| // Handler schemas declare which execution selectors they are allowed to trigger. | ||
| FunctionSchema storage handlerSchema = self.functions[handlerSelector]; | ||
| if (handlerSchema.enforceHandlerRelations) { | ||
| if (!_schemaHasHandlerSelector(handlerSchema, executionSelector)) { | ||
| revert SharedValidation.HandlerForSelectorMismatch( | ||
| executionSelector, | ||
| handlerSelector | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Strict mode still skips the role-level handler/execution pairing.
hasActionPermission() only proves the wallet has the two bitmaps somewhere, and this new branch only checks the schema-level relation. A role that intentionally narrows handlerSelector to execution A via FunctionPermission.handlerForSelectors can still be used with execution B whenever the wallet separately has B permission and the schema allows it.
As per coding guidelines, "Implement role-based access control (RBAC) with per-function permissions and protected roles".
🤖 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 2262 - 2272, The strict-mode
branch only checks the schema-level relation but misses enforcing the role-level
handler→execution pairing; update the branch where
handlerSchema.enforceHandlerRelations is checked so that after validating
_schemaHasHandlerSelector, also verify the active role's
FunctionPermission.handlerForSelectors for handlerSelector includes
executionSelector (i.e., ensure the role-specific bitmap allows that execution),
using the same role identifier/context that hasActionPermission() uses; if the
role-level mapping does not permit it, revert with
SharedValidation.HandlerForSelectorMismatch (same as current) — reference
FunctionSchema, handlerSchema.enforceHandlerRelations, hasActionPermission(),
FunctionPermission.handlerForSelectors, and _schemaHasHandlerSelector to locate
the code to change.
| return { | ||
| ...metaTx, | ||
| signature: signature.signature, | ||
| signature: signatureHex, | ||
| message: messageHash |
There was a problem hiding this comment.
This return-shape change breaks existing sanity callers.
signMetaTransaction() now returns a full meta-tx with signature as a top-level hex string, but the provided callers in scripts/sanity/secure-ownable/eip712-signing-tests.cjs and scripts/sanity/secure-ownable/broadcaster-update-tests.cjs still read signature.signature. Those flows will fail before they can execute the meta-transaction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sanity/utils/eip712-signing.cjs` around lines 186 - 189, The change
to signMetaTransaction() returned shape broke callers expecting
signature.signature; revert or provide the previous nested shape by returning
the metaTx plus a signature object containing the hex and message (e.g. keep
signMetaTransaction(...) returning { ...metaTx, signature: { signature:
signatureHex, message: messageHash } }) so existing callers that read
signature.signature continue to work, or alternatively update those callers to
read signature (but prefer restoring the nested shape in signMetaTransaction to
maintain backward compatibility).
This commit updates the `_validateExecutionAndHandlerPermissions` function within the EngineBlox library to improve the clarity and robustness of permission checks for execution and handler selectors. The documentation has been refined to emphasize the strict mode's enforcement of schema-level relationships between selectors, ensuring that the execution selector is appropriately validated against the handler's schema. These enhancements contribute to a more secure and efficient permission validation process, reinforcing the integrity of the meta-transaction handling framework.
feat: enhance permission validation in EngineBlox library
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation