Conversation
This commit refactors the function protection validation logic in the `EngineBlox` library by removing the `functionSignature` parameter from the `_validateContractFunctionProtection` function and its associated error. The changes streamline the validation process, ensuring that only the `functionSelector` is checked for protection status. Additionally, updates are made to the ABI files and TypeScript interfaces to reflect this simplification, enhancing code clarity and maintainability. These modifications align with ongoing efforts to optimize the contract's security mechanisms.
This commit updates the `updateBroadcasterRequest` and `executeBroadcasterUpdate` functions in the `SecureOwnable` contract to include a `location` parameter, allowing for more precise management of broadcaster addresses. The changes ensure that the broadcaster can be updated at a specific index in the authorized wallets set, improving flexibility and control over broadcaster assignments. Additionally, the corresponding interface and definitions have been updated to reflect this enhancement, aligning with ongoing efforts to optimize contract functionality and maintain robust security practices.
…files This commit increments the version number to 1.0.0-alpha.4 in the `package.json`, `package-lock.json`, and SDK-related package files. The changes ensure consistency across the project and prepare for the next release cycle, aligning with ongoing version management efforts to maintain clarity and organization in the codebase.
📝 WalkthroughWalkthroughAdds a uint256 Changes
Sequence Diagram(s)(Skipped — changes are API/signature and selector updates without a new multi-component sequential flow that benefits from a diagram.) 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
scripts/sanity/guard-controller/guard-controller-tests.cjs (1)
21-21:⚠️ Potential issue | 🟡 MinorStale step description: still says "Deposit native tokens."
Line 21 still reads
' 3. Deposit native tokens from owner wallet to contract', which doesn't reflect the "direct transfer" wording used everywhere else in this PR. Consider updating for consistency.Suggested fix
- console.log(' 3. Deposit native tokens from owner wallet to contract'); + console.log(' 3. Transfer native tokens directly to contract via receive()');abi/SecureBlox.abi.json (1)
290-307:⚠️ Potential issue | 🔴 CriticalLocation parameter update is consistent, but error removal is incomplete across ABI files.
All 12 ABI files with
executeBroadcasterUpdateandupdateBroadcasterRequestfunctions have been updated with thelocationparameter. However,InvalidAddresserror definitions remain in 3 files (SimpleVault.abi.json, SimpleRWA20.abi.json, and EngineBlox.abi.json) while they were removed in others (SecureBlox, RoleBlox, SecureOwnable, AccountBlox). Complete the error removal in SimpleVault and SimpleRWA20 to maintain consistency across the codebase.scripts/sanity-sdk/secure-ownable/meta-tx-execution-tests.ts (1)
95-102:⚠️ Potential issue | 🟡 Minor
findUnusedWalletmay return a wallet assigned to another role (owner, recovery).The method only excludes the current broadcaster address, but the returned wallet could collide with
ownerorrecovery, which might cause the broadcaster update to be rejected on-chain if the contract validates against existing role holders. Consider excluding all role addresses.🛡️ Suggested fix
- private findUnusedWallet(excludeAddress: Address): Address { + private findUnusedWallet(excludeAddress: Address): Address { + const excludeSet = new Set([ + excludeAddress.toLowerCase(), + this.roles.broadcaster.toLowerCase(), + ...(this.roles.owner ? [this.roles.owner.toLowerCase()] : []), + ...(this.roles.recovery ? [this.roles.recovery.toLowerCase()] : []), + ]); for (const wallet of Object.values(this.wallets)) { - if (wallet.address.toLowerCase() !== excludeAddress.toLowerCase()) { + if (!excludeSet.has(wallet.address.toLowerCase())) { return wallet.address; } } throw new Error('No unused wallet found'); }contracts/core/security/SecureOwnable.sol (1)
365-396:⚠️ Potential issue | 🟡 Minor
_updateBroadcaster: Case 1 silently no-ops when revoking at an empty slot.When
newBroadcaster == address(0)andoldBroadcaster == address(0)(no wallet atlocation), the function returns without reverting or logging. The transaction will be marked COMPLETED even though nothing changed. Consider whether this silent no-op is intentional or whether it should revert to signal the owner that the revocation target was already empty.Proposed fix (if a revert is preferred)
// Case 1: Revoke existing broadcaster at location if (newBroadcaster == address(0)) { if (oldBroadcaster != address(0)) { _revokeWallet(EngineBlox.BROADCASTER_ROLE, oldBroadcaster); _logComponentEvent(abi.encode(oldBroadcaster, address(0))); + } else { + revert SharedValidation.ResourceNotFound(EngineBlox.BROADCASTER_ROLE); } return; }
🤖 Fix all issues with AI agents
In `@contracts/examples/templates/AccountBlox.sol`:
- Around line 57-63: The receive() handler currently accepts ETH
silently—declare a receive event (e.g., event ReceivedETH(address indexed from,
uint256 amount);) and emit it inside the receive() function (emit
ReceivedETH(msg.sender, msg.value);) so every plain ETH transfer to the contract
is recorded for auditability; add the event near other contract events and
update the receive() body to emit it while keeping the function payable and
without external calls.
In `@test/foundry/unit/SecureOwnable.t.sol`:
- Around line 234-260: Add fuzz and invariant coverage for the new
location-based revoke path: create a fuzz test (e.g.,
testFuzz_UpdateBroadcasterRequest_RevokeAtLocation(uint256 location)) that seeds
broadcaster list to at least two entries, clamps/location via bound(location, 0,
broadcasters.length-1), submits a revoke-at-location request with address(0) and
advances/approves, then assert post-conditions (length decreased by one,
remaining elements equal to expected removal-at-index behavior, no duplicates,
and owner protection prevents removing last broadcaster). Also add an invariant
harness (e.g., invariant_BroadcasterListSafety) that runs across fuzzed
sequences asserting broadcasters.length >= 1, broadcasters.length <=
MAX_BROADCASTERS if applicable, no duplicate addresses, and that total
broadcaster identities are conserved except when valid revokes occur; reference
the functions secureBlox.updateBroadcasterRequest,
secureBlox.updateBroadcasterDelayedApproval, secureBlox.getBroadcasters and the
existing test_UpdateBroadcasterRequest_RevokeAtLocation_ZeroAddress for
placement and semantics.
🧹 Nitpick comments (7)
scripts/sanity/guard-controller/guard-controller-tests.cjs (1)
952-952: Consider extracting the hardcoded gas limit.The
gas: 100000magic number could be a named constant (e.g.,RECEIVE_GAS_LIMIT) for clarity and easy adjustment ifreceive()logic changes later.package.json (1)
3-3: Version bump toalpha.4is consistent across all references.The version field and all three script tags (
release:sync-versions:tag:alpha,publish:contracts,publish:sdk) consistently referencealpha.4.One consideration: using
alpha.4as an npm dist-tag (via--tag alpha.4) means each alpha release creates a separate dist-tag. Consumers installing vianpm install <pkg>@alphawon't get this version — they'd need@alpha.4explicitly. If you intend for analphadist-tag to always point to the latest alpha, consider also publishing with--tag alphaor adding a follow-upnpm dist-tag add <pkg>@1.0.0-alpha.4 alphastep.Also applies to: 34-34, 40-41
test/foundry/fuzz/SecureOwnableFuzz.t.sol (1)
49-66: Consider fuzzing thelocationparameter as well.The
locationparameter is hardcoded to0. Since this is a new parameter, fuzzing it alongsidenewBroadcasterwould improve coverage of input validation and edge cases for the location value.♻️ Suggested change
- function testFuzz_BroadcasterUpdate(address newBroadcaster) public { + function testFuzz_BroadcasterUpdate(address newBroadcaster, uint256 location) public { vm.assume(newBroadcaster != address(0)); vm.assume(newBroadcaster != broadcaster); vm.assume(newBroadcaster != owner); vm.assume(newBroadcaster != recovery); vm.prank(owner); - EngineBlox.TxRecord memory requestTx = secureBlox.updateBroadcasterRequest(newBroadcaster, 0); + EngineBlox.TxRecord memory requestTx = secureBlox.updateBroadcasterRequest(newBroadcaster, location);As per coding guidelines, "Write fuzz tests for input validation and edge cases".
test/foundry/security/AccessControl.t.sol (1)
76-78: Call site correctly updated; consider specifying the expected revert selector.The
vm.expectRevert()on Line 76 doesn't specify an error selector, unlike other tests in this file (e.g., Lines 21, 37, 83) which useabi.encodeWithSelector(...). This makes the test less precise — it will pass on any revert rather than confirming the specific access control error.sdk/typescript/utils/interface-ids.tsx (1)
33-102: Consider adding a test that cross-validates these computed interface IDs against the Solidity-computed values.A mismatch between the TypeScript-computed IDs and Solidity's
type(IInterface).interfaceIdwould silently breaksupportsInterfacechecks at runtime. The comment on lines 26-28 already notes this risk. A dedicated integration test that asserts equality between the two would catch drift early.scripts/sanity/secure-ownable/broadcaster-update-tests.cjs (1)
477-494:findUnusedWalletForBroadcasterdoesn't check against the owner or recovery addresses.The function finds a wallet different from the current broadcaster but doesn't exclude addresses currently assigned to the owner or recovery roles. If
wallet1happens to be the owner, this could assign the same address to both broadcaster and owner roles, potentially causing test failures or unexpected behavior.This is likely fine for a controlled sanity test environment where role assignments are known, but worth noting.
contracts/core/security/SecureOwnable.sol (1)
159-163: NatSpec accurately documents zero-address revocation, but consider documenting the no-op edge case.The
@paramdocs mention "zero address to revoke at location", but don't mention what happens if the location is empty. A brief note like "revocation of an empty slot is a no-op" would improve clarity for integrators.
| /** | ||
| * @dev Explicit deposit function for ETH deposits | ||
| * @notice Users must call this function to deposit ETH to the contract | ||
| * @notice Direct ETH transfers to the contract will revert (no receive() function) | ||
| * @dev Accepts plain ETH transfers (no calldata). | ||
| * @notice General-use wallet: ETH can be sent naturally; balance is credited. | ||
| * @custom:security No external calls—reentrancy-safe; outgoing ETH only via GuardController execution. | ||
| */ | ||
| event EthReceived(address indexed from, uint256 amount); | ||
|
|
||
| function deposit() external payable { | ||
| emit EthReceived(msg.sender, msg.value); | ||
| // ETH is automatically added to contract balance | ||
| } | ||
|
|
||
| receive() external payable {} | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n contracts/examples/templates/AccountBlox.solRepository: PracticalParticle/Bloxchain-Protocol
Length of output: 3656
🏁 Script executed:
rg -n --type=sol "contract\s+AccountBlox\b" contracts/examples/templates/AccountBlox.sol -A 5Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 107
🏁 Script executed:
rg -n --type=sol "ReentrancyGuard|receive\(\)|fallback\(\)|event.*Eth" contracts/examples/templates/AccountBlox.solRepository: PracticalParticle/Bloxchain-Protocol
Length of output: 107
🏁 Script executed:
rg -n "ReentrancyGuard" contracts/core/execution/GuardController.sol contracts/core/access/RuntimeRBAC.sol contracts/core/security/SecureOwnable.sol contracts/core/base/BaseStateMachine.solRepository: PracticalParticle/Bloxchain-Protocol
Length of output: 785
🏁 Script executed:
rg -n "event.*Received|event.*Eth" contracts/examples/templates/AccountBlox.sol contracts/core/execution/GuardController.sol contracts/core/access/RuntimeRBAC.sol contracts/core/security/SecureOwnable.solRepository: PracticalParticle/Bloxchain-Protocol
Length of output: 62
🏁 Script executed:
grep -n "nonReentrant" contracts/core/execution/GuardController.sol contracts/core/access/RuntimeRBAC.sol contracts/core/security/SecureOwnable.solRepository: PracticalParticle/Bloxchain-Protocol
Length of output: 62
🏁 Script executed:
grep -n "class\|contract" contracts/core/execution/GuardController.sol | head -5
grep -n "class\|contract" contracts/core/access/RuntimeRBAC.sol | head -5
grep -n "class\|contract" contracts/core/security/SecureOwnable.sol | head -5Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 1201
🏁 Script executed:
grep -n "extends\|is\|BaseStateMachine" contracts/core/execution/GuardController.sol | head -2
grep -n "extends\|is\|BaseStateMachine" contracts/core/access/RuntimeRBAC.sol | head -2
grep -n "extends\|is\|BaseStateMachine" contracts/core/security/SecureOwnable.sol | head -2Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 454
🏁 Script executed:
head -50 contracts/core/execution/GuardController.sol | tail -20
head -50 contracts/core/access/RuntimeRBAC.sol | tail -20
head -50 contracts/core/security/SecureOwnable.sol | tail -20Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 2679
🏁 Script executed:
grep -n "nonReentrant\|function.*execute" contracts/core/execution/GuardController.sol | head -20Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 719
🏁 Script executed:
grep -n "function execute" contracts/core/base/BaseStateMachine.sol -A 3Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 62
🏁 Script executed:
grep -n "function executeWithTimeLock\|nonReentrant" contracts/core/execution/GuardController.sol -A 2Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 172
🏁 Script executed:
grep -n "function.*payable\|receive\|fallback" contracts/core/base/BaseStateMachine.solRepository: PracticalParticle/Bloxchain-Protocol
Length of output: 62
🏁 Script executed:
grep -n "nonReentrant" contracts/core/execution/GuardController.solRepository: PracticalParticle/Bloxchain-Protocol
Length of output: 62
🏁 Script executed:
grep -rn "nonReentrant" contracts/core/ --include="*.sol"Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 439
🏁 Script executed:
grep -n "@notice\|function " contracts/core/base/BaseStateMachine.sol | grep -A1 "nonReentrant\|execute"Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 325
🏁 Script executed:
sed -n '116,140p' contracts/core/execution/GuardController.solRepository: PracticalParticle/Bloxchain-Protocol
Length of output: 853
Emit an event for ETH receipt to maintain auditability.
The receive() function accepts ETH silently without emitting an event. Add an event emission for audit trail purposes, as per the coding guideline: "Emit events for critical state changes."
Note: The function is not protected by nonReentrant because it performs no external calls. The comment in the function documentation correctly notes reentrancy-safety is achieved through the constraint that outgoing ETH flows only via GuardController execution (which is protected by nonReentrant on internal functions).
Proposed change for the receive handler
+ event EthReceived(address indexed from, uint256 amount);
+
/**
* `@dev` Accepts plain ETH transfers (no calldata).
* `@notice` General-use wallet: ETH can be sent naturally; balance is credited.
* `@custom`:security No external calls—reentrancy-safe; outgoing ETH only via GuardController execution.
*/
receive() external payable {
+ emit EthReceived(msg.sender, msg.value);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @dev Explicit deposit function for ETH deposits | |
| * @notice Users must call this function to deposit ETH to the contract | |
| * @notice Direct ETH transfers to the contract will revert (no receive() function) | |
| * @dev Accepts plain ETH transfers (no calldata). | |
| * @notice General-use wallet: ETH can be sent naturally; balance is credited. | |
| * @custom:security No external calls—reentrancy-safe; outgoing ETH only via GuardController execution. | |
| */ | |
| event EthReceived(address indexed from, uint256 amount); | |
| function deposit() external payable { | |
| emit EthReceived(msg.sender, msg.value); | |
| // ETH is automatically added to contract balance | |
| } | |
| receive() external payable {} | |
| event EthReceived(address indexed from, uint256 amount); | |
| /** | |
| * `@dev` Accepts plain ETH transfers (no calldata). | |
| * `@notice` General-use wallet: ETH can be sent naturally; balance is credited. | |
| * `@custom`:security No external calls—reentrancy-safe; outgoing ETH only via GuardController execution. | |
| */ | |
| receive() external payable { | |
| emit EthReceived(msg.sender, msg.value); | |
| } |
🤖 Prompt for AI Agents
In `@contracts/examples/templates/AccountBlox.sol` around lines 57 - 63, The
receive() handler currently accepts ETH silently—declare a receive event (e.g.,
event ReceivedETH(address indexed from, uint256 amount);) and emit it inside the
receive() function (emit ReceivedETH(msg.sender, msg.value);) so every plain ETH
transfer to the contract is recorded for auditability; add the event near other
contract events and update the receive() body to emit it while keeping the
function payable and without external calls.
…reOwnable This commit introduces a fuzz test for the `updateBroadcasterRequest` function, specifically targeting the revoke-at-location path. It ensures that the broadcaster list behaves correctly when revoking entries, including checks for length, index shifting, and duplicate prevention. Additionally, an invariant test is added to verify the safety of the broadcaster list, confirming that it maintains the required constraints on length and uniqueness. These enhancements improve the robustness of the test suite and align with ongoing efforts to ensure comprehensive coverage of broadcaster management functionalities.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Version Updates