Omniscia Sova Network Audit
RulesEngine Manual Review Findings
RulesEngine Manual Review Findings
REE-01M: Inexistent Truncation of Active ID Array
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | RulesEngine.sol:L326 |
Description:
The RulesEngine::_getSortedActiveHookIds function will yield a sortedActiveIds array that is filled with zero-value entries in place of any inactive hook IDs.
This can result in significant misbehaviours should a hook ID of 0 be registered in the system.
Impact:
If a hook ID of zero is registered in the system that is active, the sub hook evaluation mechanism would repetitively invoke it incorrectly.
Example:
291function _getSortedActiveHookIds() private view returns (bytes32[] memory) {292 uint256 activeHooksCount = 0;293 uint256 numHooks = _hookIds.length;294 bytes32[] memory sortedActiveIds = new bytes32[](numHooks);295 uint256 currentIndex = 0;296
297 // Count active hooks298 for (uint256 i = 0; i < numHooks;) {299 if (_hooks[_hookIds[i]].active) {300 activeHooksCount++;301 sortedActiveIds[currentIndex++] = _hookIds[i];302 }303
304 unchecked {305 ++i;306 }307 }308
309 // Simple insertion sort by priority on the active hooks310 for (uint256 i = 1; i < activeHooksCount;) {311 bytes32 key = sortedActiveIds[i];312 uint256 keyPriority = _hooks[key].priority;313 uint256 j = i;314
315 while (j > 0 && _hooks[sortedActiveIds[j - 1]].priority > keyPriority) {316 sortedActiveIds[j] = sortedActiveIds[j - 1];317 j--;318 }319 sortedActiveIds[j] = key;320
321 unchecked {322 ++i;323 }324 }325
326 return sortedActiveIds;327}Recommendation:
We advise the code to instead truncate the sortedActiveIds to the desired array length by utilizing a direct mstore operation via assembly; a practice that is confirmed to be safe as it reduces rather than increases the memory space the function uses whilst the compiler assumes that the relevant memory bytes are in use.
Alleviation (e4d6885477438291b0d3ca477fab7e088522967b):
The code was updated to properly truncate the sortedActiveIds array length, optimizing the code' gas cost significantly and addressing the edge case outlined in the exhibit.
REE-02M: Inexistent Hook Access Control
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | RulesEngine.sol: • I-1: L205-L212 • I-2: L217-L224 • I-3: L229-L236 • I-4: L259-L261 |
Description:
The RulesEngine::_evaluateSubHooks function indicates that it is meant to perform a low-level call operation instead of a staticcall operation as a hook should be able to modify its state.
The current approach is incorrect as the various before function hooks do not apply any access control, permitting any user to invoke them directly and thus cause any sub hook that relies on state changes to be corrupted.
Impact:
While normally of major severity, the absence of a stateful hook in the current implementation of the system diminishes the impact of this vulnerability to a future iteration or hook introduction.
Example:
247function _evaluateSubHooks(bytes memory callData) internal returns (IHook.HookOutput memory) {248 bytes32[] memory sortedHookIds = _getSortedActiveHookIds();249
250 for (uint256 i = 0; i < sortedHookIds.length;) {251 bytes32 hookId = sortedHookIds[i];252 HookInfo memory hook = _hooks[hookId];253
254 // Check hook is active, if not, break loop - we've processed all active hooks255 if (!hook.active) {256 break;257 }258
259 // Call the sub-hook with the appropriate evaluation function - use call instead of staticcall260 // to allow hooks to modify state (emit events, track interactions, etc.)261 (bool success, bytes memory returnData) = hook.hookAddress.call(callData);262
263 if (!success) {264 // Sub-hook execution failed (reverted without a known bytes4 selector)265 // Attempt to decode a string reason if possible, otherwise generic failure.266 // This part is tricky as revert reasons are not always returned or standard.267 // For now, let's assume a specific error selector if the call itself fails.268 revert HookEvaluationFailed(hookId, bytes4(0)); // Generic failure selector269 }270
271 // Decode the hook output from response272 IHook.HookOutput memory hookOutput = abi.decode(returnData, (IHook.HookOutput));273
274 if (!hookOutput.approved) {275 return hookOutput;276 }277
278 unchecked {279 ++i;280 }281 }282
283 // If we made it through all hooks, operation is allowed284 return IHook.HookOutput({approved: true, reason: ""});285}Recommendation:
We advise either access control to be applied on the various before hooks or their mutability to be adjusted to view, either of which we consider an adequate alleviation of this exhibit.
Alleviation (e4d6885477):
The Sova Network team evaluated this exhibit and opted to acknowledge the risk, introducing a note to the BaseHook implementation denoting that access control should be considered if stateful hook implementations are introduced.
We would like to note that this results in a slight contradiction of the code as the relevant functions should be explicitly marked as view in the current state so as to avoid misleading hook developers.
Alleviation (08da17ef72):
The Sova Network team evaluated this exhibit and specified that dependencies are expected to implement mutable functions.
As a result, we consider this exhibit safely acknowledged based on the fact that the Sova Network team is aware all mutable hooks should implement some form of access control.