Omniscia Steer Protocol Audit
Orchestrator Manual Review Findings
Orchestrator Manual Review Findings
ORO-01M: Inexistent Initialization of Base Implementation
| Type | Severity | Location |
|---|---|---|
| Language Specific |  | Orchestrator.sol:L18 |
Description:
The contract does not properly initialize the base logic implementation permitting it to be taken over by a malicious party.
Impact:
While not an active security threat, it can evolve into one if any form of delegatecall capability is introduced in one of the dependencies of the contract that could cause it to invoke a selfdestruct instruction.
Example:
18contract Orchestrator is IOrchestrator, OwnableUpgradeable, UUPSUpgradeable {Recommendation:
We advise a constructor to be introduced that simply invokes the initializer modifier to ensure that the logic implementation cannot be initialized maliciously.
Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):
A constructor was introduced that properly invokes the initializer modifier and disallows initialization of the logic implementation, alleviating this exhibit in full.
ORO-02M: Improper Bitwise Operation
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | Orchestrator.sol:L93 |
Description:
The bitwise action, while adequately annotated with warnings, performs discrepantly and can cause improper behaviour to unsuspecting users that invoke voteOnAction.
Impact:
The code currently does not behave expectedly which non-tech savvy users can be misled by.
Example:
77/**78 * @dev vote (if you are a keeper) on a given action proposal79 * @param actionHash is the hash of the action to be voted on80 * @param vote is the vote to be cast. false: reject, true: approve. false only has an effect if the keeper previously voted true. It resets their vote to false.81 */82function voteOnAction(bytes32 actionHash, bool vote) public {83 // Get voter keeper license, use to construct bitmap. Revert if no license.84 uint256 license = IKeeperRegistry(keeperRegistry).checkLicense(85 msg.sender86 );87 uint256 bitmap = 1 << license;88 if (vote) {89 // Add vote to bitmap through OR90 voteBitmaps[actionHash] |= bitmap;91 } else {92 // Remove vote from bitmap through XOR. Note that this will send a vote FOR the action if the keeper had not previously voted.93 voteBitmaps[actionHash] ^= bitmap;94 }95 emit Vote(actionHash, msg.sender, vote);96}Recommendation:
We advise an AND operation to be performed with the flipped bitmap instead (i.e. &= ~bitmap) ensuring that the action adequately sets the state of the vote to false regardless of whether the keeper has previously voted or not.
Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):
The bitwise operator was upgraded as advised ensuring that the voting bitmaps are now adequately updated when a no-vote is supplied regardless of whether a previous yes vote had occurred.
ORO-03M: Inexistent Prevention of Re-Entrancy
| Type | Severity | Location |
|---|---|---|
| Logical Fault | | Orchestrator.sol:L152-L158 |
Description:
The executeAction function can be arbitrarily re-entered by itself, thus causing gas measurements to compound and leading to a disproportionate reimburseGas total.
Impact:
The vulnerability has been classified as minor given that it requires the orchestrator voters to behave maliciously, a condition considered difficult to manifest.
Example:
152function executeAction(153 address targetAddress,154 uint256 jobEpoch,155 bytes[] calldata calldatas,156 uint256[] calldata timeIndependentLengths,157 bytes32 jobHash158) external returns (ActionState) {159 // Make sure this action is approved and has not yet been executed160 bytes32 actionHash;161 if (timeIndependentLengths.length == 0) {162 // If none of the data is time-sensitive, just use passed in calldatas163 actionHash = keccak256(164 abi.encode(targetAddress, jobEpoch, calldatas)165 );166 } else {167 // If some of it is time-sensitive, create a new array using timeIndependentLengths to represent what was originally passed in, then compare that hash instead168 uint256 calldataCount = timeIndependentLengths.length;169
170 // Construct original calldatas171 bytes[] memory timeIndependentCalldatas = new bytes[](172 calldataCount173 );174 for (uint256 i; i != calldataCount; ++i) {175 timeIndependentCalldatas[i] = calldatas[176 i177 ][:timeIndependentLengths[i]];178 }179
180 // Create hash from sliced calldatas181 actionHash = keccak256(182 abi.encode(targetAddress, jobEpoch, timeIndependentCalldatas)183 );184 }185
186 // Ensure action has not yet been executed187 require(188 actions[actionHash] == ActionState.PENDING,189 "Action already executed"190 );191
192 // Make sure this action isn't illegal (must be checked here, since elsewhere the contract only knows the action hash)193 require(targetAddress != address(this), "Invalid target address");194 require(targetAddress != gasVault, "Invalid target address");195
196 // Have this keeper vote for action. This also checks that the caller is a keeper.197 voteOnAction(actionHash, true);198
199 // Check action approval status, execute accordingly.200 bool actionApproved = actionApprovalStatus(actionHash);201 if (actionApproved) {202 // Set aside gas for this action. Keeper will be reimbursed ((originalGas - [gas remaining when returnGas is called]) * gasPrice) wei.203 uint256 originalGas = gasleft();204
205 // Execute action206 (bool success, ) = address(this).call{ // Check gas available for this transaction. This call will fail if gas available is insufficient or this call's gas price is too high.207 gas: IGasVault(gasVault).gasAvailableForTransaction(208 targetAddress209 )210 }(211 abi.encodeWithSignature(212 "_executeAction(address,bytes[])",213 targetAddress,214 calldatas215 )216 );217
218 // Reimburse keeper for gas used, whether action execution succeeded or not. The reimbursement will be stored inside the GasVault.219 IGasVault(gasVault).reimburseGas(220 targetAddress,221 originalGas,222 jobHash == bytes32(0) ? actionHash : jobHash // If a jobhash was passed in, use that. Otherwise use the action hash.223 );224
225 // Record result226 if (success) {227 emit ActionExecuted(actionHash, _msgSender(), rewardPerAction);228
229 // Set state to completed230 actions[actionHash] = ActionState.COMPLETED;231
232 return ActionState.COMPLETED;233 } else {234 emit ActionFailed(actionHash);235
236 return ActionState.PENDING;237 }238 } else {239 // If action is not approved, revert.240 revert("Votes lacking; state still pending");241 }242}Recommendation:
We advise the function to be marked as non-reentrant, preventing a malicious proposal from draining all the gas reimbursement allocation the contract had for itself.
Alleviation (200f275c40):
Upon discussion with the Steer Finance team, we initially assessed that the function is adequately protected against re-entrancies via the require check it performs that the target of the action is not address(this).
During the remediation round, we re-evaluated this particular finding and identified that despite this check it is still valid. If executeAction invokes an external contract (passing the != address(this) check) that in turn invokes executeAction again, the re-entrancy attack is possible and gas rewards will compound as the function marks an action as "executed" after the external call by it concludes. As a result, we consider this exhibit not alleviated and we urge the Steer Protocol team to properly guard it against re-entrancies.
As an additional note, the attack is indeed possible because a keeper can be a smart contract based on the implementation of KeeperRegistry.
Alleviation (0ed41ccc18):
After evaluating the newfound information we supplied to the Steer Protocol team, they proceeded with applying a re-entrancy protection measure in the form of the existing actions[actionHash] data entry check by marking it as COMPLETED prior to performing the external call. As a result, we consider this exhibit adequately alleviated.
ORO-04M: Action Hash Conflict
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | Orchestrator.sol:L163-L165, L181-L183 |
Description:
The way the time-dependent variable injection system works for the executeAction function is susceptible to action hash collisions which could in turn cause certain systems to misbehave (i.e. caller identification via trailing call data or inexistent supply of time-sensitive data).
Impact:
It is currently possible to sabotage a call by executing it with no timeIndependentCalldatas even if it is meant to accept some and vice-versa, execute a static call with dynamically appended data that could cause the targetAddress to misbehave if they perform special operations on msg.data (such as GSN applications).
Example:
161if (timeIndependentLengths.length == 0) {162 // If none of the data is time-sensitive, just use passed in calldatas163 actionHash = keccak256(164 abi.encode(targetAddress, jobEpoch, calldatas)165 );166} else {167 // If some of it is time-sensitive, create a new array using timeIndependentLengths to represent what was originally passed in, then compare that hash instead168 uint256 calldataCount = timeIndependentLengths.length;169
170 // Construct original calldatas171 bytes[] memory timeIndependentCalldatas = new bytes[](172 calldataCount173 );174 for (uint256 i; i != calldataCount; ++i) {175 timeIndependentCalldatas[i] = calldatas[176 i177 ][:timeIndependentLengths[i]];178 }179
180 // Create hash from sliced calldatas181 actionHash = keccak256(182 abi.encode(targetAddress, jobEpoch, timeIndependentCalldatas)183 );184}Recommendation:
We advise a special distinction to be made between dynamically constructed calls and static calls by adjoining a new constant variable between the jobEpoch and the timeIndependentCalldatas that would cause resulting action hashes to be different.
Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):
A salt member has been introduced to the actionHash construction inside the time-sensitive branch of executeAction thus addressing this exhibit in full.
ORO-05M: Inexistent Protection of Target Address
| Type | Severity | Location |
|---|---|---|
| Logical Fault | | Orchestrator.sol:L213 |
Description:
The targetAddress variable remains unsanitized, permitting the contract to impersonate itself when invoking other sensitive system modules. As an exemplary exploit, the contract can invoke gasVault directly with the reimburseGas function and empty any targetAddress allocation it wishes.
Impact:
The vulnerability has been classified as medium given that its impact is high but its exploitability is low as the orchestrator users would need to conspire and vote on this attack.
Example:
205// Execute action206(bool success, ) = address(this).call{ // Check gas available for this transaction. This call will fail if gas available is insufficient or this call's gas price is too high.207 gas: IGasVault(gasVault).gasAvailableForTransaction(208 targetAddress209 )210}(211 abi.encodeWithSignature(212 "_executeAction(address,bytes[])",213 targetAddress,214 calldatas215 )216);217
218// Reimburse keeper for gas used, whether action execution succeeded or not. The reimbursement will be stored inside the GasVault.219IGasVault(gasVault).reimburseGas(220 targetAddress,221 originalGas,222 jobHash == bytes32(0) ? actionHash : jobHash // If a jobhash was passed in, use that. Otherwise use the action hash.223);Recommendation:
We advise the targetAddress to be sanitized as different than the gasVault and any other sensitive contract the Orchestrator may interact with in the future.
Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):
After discussions with the Steer Protocol team, we identified that the targetAddress is indeed properly sanitized against sensitive targets (address(this) & gasVault) thereby rendering this exhibit nullified.