Skip to content
Merged

Dev #78

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 80 additions & 55 deletions contracts/core/access/RuntimeRBAC.sol
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,8 @@ abstract contract RuntimeRBAC is BaseStateMachine, IRuntimeRBAC {
uint256 timeLockPeriodSec,
address eventForwarder
) public virtual onlyInitializing {
// Initialize base state machine (only if not already initialized)
if (!_secureState.initialized) {
_initializeBaseStateMachine(initialOwner, broadcaster, recovery, timeLockPeriodSec, eventForwarder);
}

_initializeBaseStateMachine(initialOwner, broadcaster, recovery, timeLockPeriodSec, eventForwarder);

// Load RuntimeRBAC-specific definitions
IDefinition.RolePermission memory permissions = RuntimeRBACDefinitions.getRolePermissions();
_loadDefinitions(
Expand Down Expand Up @@ -117,68 +114,96 @@ abstract contract RuntimeRBAC is BaseStateMachine, IRuntimeRBAC {
IRuntimeRBAC.RoleConfigAction calldata action = actions[i];

if (action.actionType == IRuntimeRBAC.RoleConfigActionType.CREATE_ROLE) {
// Decode CREATE_ROLE action data
// Format: (string roleName, uint256 maxWallets)
(
string memory roleName,
uint256 maxWallets
) = abi.decode(action.data, (string, uint256));

// Create the role in the secure state with isProtected = false
bytes32 roleHash = _createRole(roleName, maxWallets, false);

_logComponentEvent(_encodeRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.CREATE_ROLE, roleHash, bytes4(0)));
_executeCreateRole(action.data);
} else if (action.actionType == IRuntimeRBAC.RoleConfigActionType.REMOVE_ROLE) {
// Decode REMOVE_ROLE action data
// Format: (bytes32 roleHash)
(bytes32 roleHash) = abi.decode(action.data, (bytes32));
_removeRole(roleHash);

_logComponentEvent(_encodeRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.REMOVE_ROLE, roleHash, bytes4(0)));
_executeRemoveRole(action.data);
} else if (action.actionType == IRuntimeRBAC.RoleConfigActionType.ADD_WALLET) {
// Decode ADD_WALLET action data
// Format: (bytes32 roleHash, address wallet)
(bytes32 roleHash, address wallet) = abi.decode(action.data, (bytes32, address));
_requireRoleNotProtected(roleHash);
_assignWallet(roleHash, wallet);

_logComponentEvent(_encodeRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.ADD_WALLET, roleHash, bytes4(0)));
_executeAddWallet(action.data);
} else if (action.actionType == IRuntimeRBAC.RoleConfigActionType.REVOKE_WALLET) {
// Decode REVOKE_WALLET action data
// Format: (bytes32 roleHash, address wallet)
(bytes32 roleHash, address wallet) = abi.decode(action.data, (bytes32, address));
_requireRoleNotProtected(roleHash);
_revokeWallet(roleHash, wallet);

_logComponentEvent(_encodeRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.REVOKE_WALLET, roleHash, bytes4(0)));
_executeRevokeWallet(action.data);
} else if (action.actionType == IRuntimeRBAC.RoleConfigActionType.ADD_FUNCTION_TO_ROLE) {
// Decode ADD_FUNCTION_TO_ROLE action data
// Format: (bytes32 roleHash, FunctionPermission functionPermission)
(
bytes32 roleHash,
EngineBlox.FunctionPermission memory functionPermission
) = abi.decode(action.data, (bytes32, EngineBlox.FunctionPermission));

_addFunctionToRole(roleHash, functionPermission);

_logComponentEvent(_encodeRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.ADD_FUNCTION_TO_ROLE, roleHash, functionPermission.functionSelector));
_executeAddFunctionToRole(action.data);
} else if (action.actionType == IRuntimeRBAC.RoleConfigActionType.REMOVE_FUNCTION_FROM_ROLE) {
// Decode REMOVE_FUNCTION_FROM_ROLE action data
// Format: (bytes32 roleHash, bytes4 functionSelector)
(bytes32 roleHash, bytes4 functionSelector) = abi.decode(action.data, (bytes32, bytes4));
_removeFunctionFromRole(roleHash, functionSelector);

_logComponentEvent(_encodeRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.REMOVE_FUNCTION_FROM_ROLE, roleHash, functionSelector));
_executeRemoveFunctionFromRole(action.data);
} else {
revert SharedValidation.NotSupported();
}
}
}

/**
* @dev Encodes RBAC config event payload for ComponentEvent. Decode as (RoleConfigActionType, bytes32 roleHash, bytes4 functionSelector).
* @dev Executes CREATE_ROLE: creates a new non-protected role
* @param data ABI-encoded (string roleName, uint256 maxWallets)
*/
function _executeCreateRole(bytes calldata data) internal {
(string memory roleName, uint256 maxWallets) = abi.decode(data, (string, uint256));
bytes32 roleHash = _createRole(roleName, maxWallets, false);
_logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.CREATE_ROLE, roleHash, bytes4(0), address(0));
}

/**
* @dev Executes REMOVE_ROLE: removes a role by hash
* @param data ABI-encoded (bytes32 roleHash)
*/
function _executeRemoveRole(bytes calldata data) internal {
(bytes32 roleHash) = abi.decode(data, (bytes32));
_removeRole(roleHash);
_logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.REMOVE_ROLE, roleHash, bytes4(0), address(0));
}
Comment on lines +148 to +152
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

find . -name "RuntimeRBAC.sol" -type f

Repository: PracticalParticle/Bloxchain-Protocol

Length of output: 119


🏁 Script executed:

cat -n contracts/core/access/RuntimeRBAC.sol | head -200

Repository: PracticalParticle/Bloxchain-Protocol

Length of output: 10157


🏁 Script executed:

rg -n "_requireRoleNotProtected" -A 5 --type sol -B 2

Repository: PracticalParticle/Bloxchain-Protocol

Length of output: 107


🏁 Script executed:

rg -n "_removeRole" -A 10 --type sol

Repository: PracticalParticle/Bloxchain-Protocol

Length of output: 107


🏁 Script executed:

rg -n "_removeRole" -g "*.sol" -A 10 -B 2

Repository: PracticalParticle/Bloxchain-Protocol

Length of output: 2397


🏁 Script executed:

rg -n "isProtected" -g "*.sol" -B 2 -A 2 | head -100

Repository: PracticalParticle/Bloxchain-Protocol

Length of output: 8584


🏁 Script executed:

rg -n "OWNER|BROADCASTER|RECOVERY" -g "*.sol" -B 1 -A 1 | head -80

Repository: PracticalParticle/Bloxchain-Protocol

Length of output: 5623


Add missing _requireRoleNotProtected check in _executeRemoveRole.

_executeAddWallet and _executeRevokeWallet both guard against modifying protected roles via _requireRoleNotProtected, but _executeRemoveRole is missing this check. This allows removing protected roles (OWNER, BROADCASTER, RECOVERY) through batch operations, bypassing the design intent stated in the function's docstring and creating a critical access-control vulnerability.

Proposed fix
     function _executeRemoveRole(bytes calldata data) internal {
         (bytes32 roleHash) = abi.decode(data, (bytes32));
+        _requireRoleNotProtected(roleHash);
         _removeRole(roleHash);
         _logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.REMOVE_ROLE, roleHash, bytes4(0), address(0));
     }
📝 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.

Suggested change
function _executeRemoveRole(bytes calldata data) internal {
(bytes32 roleHash) = abi.decode(data, (bytes32));
_removeRole(roleHash);
_logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.REMOVE_ROLE, roleHash, bytes4(0), address(0));
}
function _executeRemoveRole(bytes calldata data) internal {
(bytes32 roleHash) = abi.decode(data, (bytes32));
_requireRoleNotProtected(roleHash);
_removeRole(roleHash);
_logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.REMOVE_ROLE, roleHash, bytes4(0), address(0));
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/core/access/RuntimeRBAC.sol` around lines 148 - 152, The
_executeRemoveRole function is missing the protection check that prevents
removing protected roles; add a call to _requireRoleNotProtected(roleHash) at
the start of _executeRemoveRole (before calling _removeRole and
_logRoleConfigEvent) so removals of OWNER/BROADCASTER/RECOVERY (and any other
protected roles enforced by _requireRoleNotProtected) are blocked in batch
operations; locate _executeRemoveRole, insert the same
_requireRoleNotProtected(roleHash) guard used in _executeAddWallet and
_executeRevokeWallet.


/**
* @dev Executes ADD_WALLET: assigns a wallet to a role (role must not be protected)
* @param data ABI-encoded (bytes32 roleHash, address wallet)
*/
function _executeAddWallet(bytes calldata data) internal {
(bytes32 roleHash, address wallet) = abi.decode(data, (bytes32, address));
_requireRoleNotProtected(roleHash);
_assignWallet(roleHash, wallet);
_logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.ADD_WALLET, roleHash, bytes4(0), wallet);
}

/**
* @dev Executes REVOKE_WALLET: revokes a wallet from a role (role must not be protected)
* @param data ABI-encoded (bytes32 roleHash, address wallet)
*/
function _executeRevokeWallet(bytes calldata data) internal {
(bytes32 roleHash, address wallet) = abi.decode(data, (bytes32, address));
_requireRoleNotProtected(roleHash);
_revokeWallet(roleHash, wallet);
_logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.REVOKE_WALLET, roleHash, bytes4(0), wallet);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* @dev Executes ADD_FUNCTION_TO_ROLE: adds a function permission to a role
* @param data ABI-encoded (bytes32 roleHash, FunctionPermission functionPermission)
*/
function _executeAddFunctionToRole(bytes calldata data) internal {
(
bytes32 roleHash,
EngineBlox.FunctionPermission memory functionPermission
) = abi.decode(data, (bytes32, EngineBlox.FunctionPermission));
_addFunctionToRole(roleHash, functionPermission);
_logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.ADD_FUNCTION_TO_ROLE, roleHash, functionPermission.functionSelector, address(0));
}

/**
* @dev Executes REMOVE_FUNCTION_FROM_ROLE: removes a function permission from a role
* @param data ABI-encoded (bytes32 roleHash, bytes4 functionSelector)
*/
function _executeRemoveFunctionFromRole(bytes calldata data) internal {
(bytes32 roleHash, bytes4 functionSelector) = abi.decode(data, (bytes32, bytes4));
_removeFunctionFromRole(roleHash, functionSelector);
_logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.REMOVE_FUNCTION_FROM_ROLE, roleHash, functionSelector, address(0));
}

/**
* @dev Encodes and logs a role config event via ComponentEvent. Payload decodes as (RoleConfigActionType, bytes32 roleHash, bytes4 functionSelector, address wallet).
* @param action The role config action type
* @param roleHash The role hash
* @param selector The function selector (or zero for N/A)
* @param wallet The wallet address (or zero for actions that do not apply to a wallet)
*/
function _encodeRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType action, bytes32 roleHash, bytes4 selector) internal pure returns (bytes memory) {
return abi.encode(action, roleHash, selector);
function _logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType action, bytes32 roleHash, bytes4 selector, address wallet) internal {
_logComponentEvent(abi.encode(action, roleHash, selector, wallet));
}
}
15 changes: 8 additions & 7 deletions contracts/core/base/BaseStateMachine.sol
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,15 @@ abstract contract BaseStateMachine is Initializable, ERC165Upgradeable, Reentran
uint256 timeLockPeriodSec,
address eventForwarder
) internal onlyInitializing {
__ERC165_init();
__ReentrancyGuard_init();

_secureState.initialize(initialOwner, broadcaster, recovery, timeLockPeriodSec);
// Skip if already initialized (e.g. when multiple components call from Account.initialize)
if (!_secureState.initialized) {
__ERC165_init();
__ReentrancyGuard_init();

_secureState.initialize(initialOwner, broadcaster, recovery, timeLockPeriodSec);

_secureState.setEventForwarder(eventForwarder);
_secureState.setEventForwarder(eventForwarder);
}
}

// ============ SYSTEM ROLE QUERY FUNCTIONS ============
Expand Down Expand Up @@ -294,7 +297,6 @@ abstract contract BaseStateMachine is Initializable, ERC165Upgradeable, Reentran
*/
function _setHook(bytes4 functionSelector, address hook) internal {
EngineBlox.addTargetToFunctionHooks(_getSecureState(), functionSelector, hook);
_logComponentEvent(abi.encode(functionSelector, hook));
}

/**
Expand All @@ -305,7 +307,6 @@ abstract contract BaseStateMachine is Initializable, ERC165Upgradeable, Reentran
*/
function _clearHook(bytes4 functionSelector, address hook) internal {
EngineBlox.removeTargetFromFunctionHooks(_getSecureState(), functionSelector, hook);
_logComponentEvent(abi.encode(functionSelector, hook));
}

/**
Expand Down
99 changes: 58 additions & 41 deletions contracts/core/execution/GuardController.sol
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,8 @@ abstract contract GuardController is BaseStateMachine {
uint256 timeLockPeriodSec,
address eventForwarder
) public virtual onlyInitializing {
// Initialize base state machine (only if not already initialized)
if (!_secureState.initialized) {
_initializeBaseStateMachine(initialOwner, broadcaster, recovery, timeLockPeriodSec, eventForwarder);
}

_initializeBaseStateMachine(initialOwner, broadcaster, recovery, timeLockPeriodSec, eventForwarder);

// Load GuardController-specific definitions
IDefinition.RolePermission memory guardControllerPermissions = GuardControllerDefinitions.getRolePermissions();
_loadDefinitions(
Expand Down Expand Up @@ -341,56 +338,76 @@ abstract contract GuardController is BaseStateMachine {
IGuardController.GuardConfigAction calldata action = actions[i];

if (action.actionType == IGuardController.GuardConfigActionType.ADD_TARGET_TO_WHITELIST) {
// Decode ADD_TARGET_TO_WHITELIST action data
// Format: (bytes4 functionSelector, address target)
(bytes4 functionSelector, address target) = abi.decode(action.data, (bytes4, address));

_addTargetToFunctionWhitelist(functionSelector, target);

_logComponentEvent(_encodeGuardConfigEvent(IGuardController.GuardConfigActionType.ADD_TARGET_TO_WHITELIST, functionSelector, target));
_executeAddTargetToWhitelist(action.data);
} else if (action.actionType == IGuardController.GuardConfigActionType.REMOVE_TARGET_FROM_WHITELIST) {
// Decode REMOVE_TARGET_FROM_WHITELIST action data
// Format: (bytes4 functionSelector, address target)
(bytes4 functionSelector, address target) = abi.decode(action.data, (bytes4, address));

_removeTargetFromFunctionWhitelist(functionSelector, target);

_logComponentEvent(_encodeGuardConfigEvent(IGuardController.GuardConfigActionType.REMOVE_TARGET_FROM_WHITELIST, functionSelector, target));
_executeRemoveTargetFromWhitelist(action.data);
} else if (action.actionType == IGuardController.GuardConfigActionType.REGISTER_FUNCTION) {
// Decode REGISTER_FUNCTION action data
// Format: (string functionSignature, string operationName, TxAction[] supportedActions)
(
string memory functionSignature,
string memory operationName,
EngineBlox.TxAction[] memory supportedActions
) = abi.decode(action.data, (string, string, EngineBlox.TxAction[]));

bytes4 functionSelector = _registerFunction(functionSignature, operationName, supportedActions);

_logComponentEvent(_encodeGuardConfigEvent(IGuardController.GuardConfigActionType.REGISTER_FUNCTION, functionSelector, address(0)));
_executeRegisterFunction(action.data);
} else if (action.actionType == IGuardController.GuardConfigActionType.UNREGISTER_FUNCTION) {
// Decode UNREGISTER_FUNCTION action data
// Format: (bytes4 functionSelector, bool safeRemoval)
(bytes4 functionSelector, bool safeRemoval) = abi.decode(action.data, (bytes4, bool));

_unregisterFunction(functionSelector, safeRemoval);

_logComponentEvent(_encodeGuardConfigEvent(IGuardController.GuardConfigActionType.UNREGISTER_FUNCTION, functionSelector, address(0)));
_executeUnregisterFunction(action.data);
} else {
revert SharedValidation.NotSupported();
}
}
}

/**
* @dev Encodes guard config event payload for ComponentEvent. Decode as (GuardConfigActionType, bytes4 functionSelector, address target).
* @dev Executes ADD_TARGET_TO_WHITELIST: adds a target address to a function's call whitelist
* @param data ABI-encoded (bytes4 functionSelector, address target)
*/
function _executeAddTargetToWhitelist(bytes calldata data) internal {
(bytes4 functionSelector, address target) = abi.decode(data, (bytes4, address));
_addTargetToFunctionWhitelist(functionSelector, target);
_logGuardConfigEvent(IGuardController.GuardConfigActionType.ADD_TARGET_TO_WHITELIST, functionSelector, target);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* @dev Executes REMOVE_TARGET_FROM_WHITELIST: removes a target address from a function's call whitelist
* @param data ABI-encoded (bytes4 functionSelector, address target)
*/
function _executeRemoveTargetFromWhitelist(bytes calldata data) internal {
(bytes4 functionSelector, address target) = abi.decode(data, (bytes4, address));
_removeTargetFromFunctionWhitelist(functionSelector, target);
_logGuardConfigEvent(IGuardController.GuardConfigActionType.REMOVE_TARGET_FROM_WHITELIST, functionSelector, target);
}

/**
* @dev Executes REGISTER_FUNCTION: registers a new function schema with signature, operation name, and supported actions
* @param data ABI-encoded (string functionSignature, string operationName, TxAction[] supportedActions)
*/
function _executeRegisterFunction(bytes calldata data) internal {
(
string memory functionSignature,
string memory operationName,
EngineBlox.TxAction[] memory supportedActions
) = abi.decode(data, (string, string, EngineBlox.TxAction[]));

bytes4 functionSelector = _registerFunction(functionSignature, operationName, supportedActions);
_logGuardConfigEvent(IGuardController.GuardConfigActionType.REGISTER_FUNCTION, functionSelector, address(0));
}

/**
* @dev Executes UNREGISTER_FUNCTION: unregisters a function schema by selector
* @param data ABI-encoded (bytes4 functionSelector, bool safeRemoval)
*/
function _executeUnregisterFunction(bytes calldata data) internal {
(bytes4 functionSelector, bool safeRemoval) = abi.decode(data, (bytes4, bool));
_unregisterFunction(functionSelector, safeRemoval);
_logGuardConfigEvent(IGuardController.GuardConfigActionType.UNREGISTER_FUNCTION, functionSelector, address(0));
}

/**
* @dev Encodes and logs a guard config event via ComponentEvent. Payload decodes as (GuardConfigActionType, bytes4 functionSelector, address target).
* @param actionType The guard config action type
* @param functionSelector The function selector (or zero for N/A)
* @param target The target address (or zero for N/A)
*/
function _encodeGuardConfigEvent(
function _logGuardConfigEvent(
IGuardController.GuardConfigActionType actionType,
bytes4 functionSelector,
address target
) internal pure returns (bytes memory) {
return abi.encode(actionType, functionSelector, target);
) internal {
_logComponentEvent(abi.encode(actionType, functionSelector, target));
}

// ============ INTERNAL FUNCTION SCHEMA HELPERS ============
Expand Down
2 changes: 2 additions & 0 deletions contracts/core/lib/EngineBlox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -1300,6 +1300,8 @@ library EngineBlox {
bytes4 functionSelector,
address target
) public {
SharedValidation.validateNotZeroAddress(target);

EnumerableSet.AddressSet storage set = self.functionTargetWhitelist[functionSelector];
if (!set.remove(target)) {
revert SharedValidation.ItemNotFound(target);
Expand Down
Loading