Omniscia Steer Protocol Audit
SmartRewardsDistributor Manual Review Findings
SmartRewardsDistributor Manual Review Findings
SRD-01M: Inexistent Preventions of Repetitive Cancellations
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | SmartRewardsDistributor.sol:L274-L311 |
Description:
The SmartRewardsDistributor::cancelCampaign function can be repetitively invoked an unlimited number of times, each time updating the cancelBlock to a higher number.
Impact:
While presently there is no vulnerability that arises from this behaviour, issues may arise from the application of some of our recommendations outlined in the audit report for separate exhibits.
Example:
274/// @dev cancel a campaign from distributing rewards, refund tokens to owner275/// @param campaignId the id of the campaign to cancel276/// @param halt true if all outstanding rewards should also be refunded277function cancelCampaign(uint256 campaignId, bool halt) external returns (uint256) {278
279 RewardCampaign memory campaign = rewardCampaigns[campaignId];280 require(msg.sender == campaign.owner, "Must be owner to cancel");281 require(campaign.endBlock > lastBlockUpdate, "Campaign must be active or pending");282
283 uint256 campaignSpent = campaignCredit[campaignId];284 uint256 outbound;285 // pause all withdraws286 if (halt) {287 // ALL tokens on hand refunded288 outbound = campaign.amount - campaignSpent;289 campaignCredit[campaignId] = campaign.amount;290 IERC20(campaign.rewardToken).transfer(291 campaign.owner,292 outbound293 );294 pausedCampaigns[campaignId] = true;295 }296 else {297 // Only tokens not allocated refunded298 uint256 totalAllocated = getAvailableFundsUpToBlock(campaign, lastBlockUpdate);299 outbound = campaign.amount - totalAllocated;300 campaignCredit[campaignId] = campaignSpent + outbound;301 IERC20(campaign.rewardToken).transfer(302 campaign.owner,303 outbound304 );305 }306 // mark campaign as canceled307 rewardCampaigns[campaignId].cancelBlock = lastBlockUpdate;308
309 emit CampaignCanceled(campaignId, halt, outbound, msg.sender);310 return outbound;311}Recommendation:
We advise this to be prohibited as it will interfere with proposed alleviations for other exhibits outlined in the audit report.
Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):
The SmartRewardsDistributor::cancelCampaign function properly imposes a require check preventing the same campaign from being cancelled twice, alleviating this exhibit.
SRD-02M: Inexistent Validation of Liquidity Pool
| Type | Severity | Location |
|---|---|---|
| Input Sanitization | | SmartRewardsDistributor.sol:L234 |
Description:
The SmartRewardsDistributor contract permits anyone to create a reward campaign, and the liquidityPool that is specified for the reward campaign is unsanitized.
This permits a malicious user to create a lucrative campaign for a liquidityPool that would scam users, effectively weaponizing the reward system as bait.
Impact:
The severity of this exhibit cannot be quantified as it pertains to how the liquidityPool data point is utilized off-chain by the Steer Protocol.
Example:
232rewardCampaigns[campaignId] = RewardCampaign({233 owner: newRewardCampaign.owner,234 liquidityPool: newRewardCampaign.liquidityPool,235 rewardToken: newRewardCampaign.rewardToken,236 amount: distributionRewards,237 startBlock: newRewardCampaign.startBlock,238 endBlock: newRewardCampaign.endBlock,239 campaignParamsIPFSHash: newRewardCampaign.campaignParamsIPFSHash,240 abandonmentDeadlineBlock: abandonDeadline,241 cancelBlock: 0242});243emit RewardCampaignCreated(244 campaignId,245 newRewardCampaign.owner,246 newRewardCampaign.startBlock,247 newRewardCampaign.endBlock,248 newRewardCampaign.liquidityPool,249 newRewardCampaign.rewardToken,250 distributionRewards,251 newRewardCampaign.campaignParamsIPFSHash,252 abandonDeadline253);Recommendation:
We advise the liquidityPool values to be sanitized as part of a whitelist. Alternatively, we advise a signature-based scheme to avoid the gas costs involved with maintaining all potential valid liquidity pools users can set as part of their campaigns.
As yet another approach, campaigns could undergo a "proposal" phase in which anyone can submit them, permitting them to be activated by interaction from a Steer Protocol affiliated party.
Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):
The Steer Protocol team evaluated this exhibit and clarified that validation of the liquidity pool on-chain would be complex, and that they wish to apply this validation off-chain before updating a Merkle root to include rewards for the relevant campaign.
As such, we consider this exhibit safely acknowledged based on the fact that the off-chain code of the Steer Protocol will properly sanitize these addresses.
SRD-03M: Inexistent Emission of Event
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | SmartRewardsDistributor.sol:L294, L364 |
Description:
The CampaignPaused event is not emitted when the pause status of a campaign is altered as a result of a SmartRewardsDistributor::cancelCampaign operation.
Impact:
As this change purely relates to off-chain impact, its severity cannot be assessed as higher than informational.
Example:
274/// @dev cancel a campaign from distributing rewards, refund tokens to owner275/// @param campaignId the id of the campaign to cancel276/// @param halt true if all outstanding rewards should also be refunded277function cancelCampaign(uint256 campaignId, bool halt) external returns (uint256) {278
279 RewardCampaign memory campaign = rewardCampaigns[campaignId];280 require(msg.sender == campaign.owner, "Must be owner to cancel");281 require(campaign.endBlock > lastBlockUpdate, "Campaign must be active or pending");282
283 uint256 campaignSpent = campaignCredit[campaignId];284 uint256 outbound;285 // pause all withdraws286 if (halt) {287 // ALL tokens on hand refunded288 outbound = campaign.amount - campaignSpent;289 campaignCredit[campaignId] = campaign.amount;290 IERC20(campaign.rewardToken).transfer(291 campaign.owner,292 outbound293 );294 pausedCampaigns[campaignId] = true;295 }296 else {297 // Only tokens not allocated refunded298 uint256 totalAllocated = getAvailableFundsUpToBlock(campaign, lastBlockUpdate);299 outbound = campaign.amount - totalAllocated;300 campaignCredit[campaignId] = campaignSpent + outbound;301 IERC20(campaign.rewardToken).transfer(302 campaign.owner,303 outbound304 );305 }306 // mark campaign as canceled307 rewardCampaigns[campaignId].cancelBlock = lastBlockUpdate;308
309 emit CampaignCanceled(campaignId, halt, outbound, msg.sender);310 return outbound;311}Recommendation:
We advise the event to be properly emitted, ensuring consistency in the off-chain outputs of the contract.
Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):
The CampaignPaused event is correctly emitted whenever a halt operation occurs, addressing this exhibit and ensuring off-chain processes can adequately capture the campaign's pause.
SRD-04M: Non-Standard Disable of Initializers
| Type | Severity | Location |
|---|---|---|
| Standard Conformity | | SmartRewardsDistributor.sol:L80 |
Description:
The Initializable::initializer modifier is utilized in the SmartRewardsDistributor::constructor to disable the initializer at the base implementation, however, this is non-standard as the re-initialization pattern could still be utilized in the base implementation in theory.
Impact:
As initialization is still prohibited, the proposed pattern is a matter of best practice rather than an actual vulnerability.
Example:
79/// @custom:oz-upgrades-unsafe-allow constructor80constructor() initializer() {}81
82function initialize(83 address _orchestratorRelay,84 uint256 _steerFee,85 address _feeCollectionAddress,86 uint256 _abandonmentBlockTimeframe87) public initializer {Recommendation:
We advise the Initializable::_disableInitializers function to be utilized instead, ensuring that the logic implementation is correctly disabled.
Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):
The Steer Protocol team evaluated this exhibit and clarified that they do not utilize an OpenZeppelin version that exposes the Initializable::_disableInitializers function.
As such, the presently adopted pattern by the Steer Protocol team is correct rendering this exhibit nullified.
SRD-05M: Misleading Function Name
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | SmartRewardsDistributor.sol:L355 |
Description:
The SmartRewardsDistributor::pauseCampaign function, despite what its name implies, can be utilized to toggle the state of a campaign from a paused to an unpaused status and vice versa.
Impact:
Toggling functions are generally insecure as they rely on transaction ordering, and the present SmartRewardsDistributor::pauseCampaign function is misleading as to its purpose.
Example:
354/// @dev for emergencies or misRegisters, cancel campaign should be used in most circumstances355function pauseCampaign(uint campaignId) external returns (bool) {356 RewardCampaign memory campaign = rewardCampaigns[campaignId];357 require(msg.sender == campaign.owner || msg.sender == owner(), "Must be campaign owner to pause");358 bool state = !pausedCampaigns[campaignId];359 if (campaign.cancelBlock != 0 && !state) {360 // Pausing a canceled campaign locks it, cannot be unpaused361 revert("Cannot unpause canceled campaign!");362 }363 pausedCampaigns[campaignId] = state;364 emit CampaignPaused(campaignId, msg.sender, state);365 return state;366}Recommendation:
We advise the function to be revised, accepting an argument for the desired campaign state instead as toggling functions rely on transaction ordering which we consider insecure.
Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):
The function's name has been adequately updated to reflect that it is a toggle-based function, addressing this exhibit.
SRD-06M: Potential Desynchronization of Merkle Root Management
| Type | Severity | Location |
|---|---|---|
| Language Specific |  | SmartRewardsDistributor.sol:L41, L158-L159, L307 |
Description:
The current Merkle root system in use by the SmartRewardsDistributor is insecure due to amalgamating all campaign claims under a single root. Apart from scalability issues in terms of gas costs, another flaw that arises from this approach is the fact that a campaign's cancellation will not necessarily correlate to the Merkle root's state as their synchronous nature is entirely delegated to the orchestrator's timely operation.
Impact:
An inconsistency between a campaign's cancellation and the Merkle root submitted by the Orchestrator will result in claimants receiving discrepant amounts from the campaign even if they would normally acquire them if the cancellation was enforced for the Merkle root as well.
Example:
274/// @dev cancel a campaign from distributing rewards, refund tokens to owner275/// @param campaignId the id of the campaign to cancel276/// @param halt true if all outstanding rewards should also be refunded277function cancelCampaign(uint256 campaignId, bool halt) external returns (uint256) {278
279 RewardCampaign memory campaign = rewardCampaigns[campaignId];280 require(msg.sender == campaign.owner, "Must be owner to cancel");281 require(campaign.endBlock > lastBlockUpdate, "Campaign must be active or pending");282
283 uint256 campaignSpent = campaignCredit[campaignId];284 uint256 outbound;285 // pause all withdraws286 if (halt) {287 // ALL tokens on hand refunded288 outbound = campaign.amount - campaignSpent;289 campaignCredit[campaignId] = campaign.amount;290 IERC20(campaign.rewardToken).transfer(291 campaign.owner,292 outbound293 );294 pausedCampaigns[campaignId] = true;295 }296 else {297 // Only tokens not allocated refunded298 uint256 totalAllocated = getAvailableFundsUpToBlock(campaign, lastBlockUpdate);299 outbound = campaign.amount - totalAllocated;300 campaignCredit[campaignId] = campaignSpent + outbound;301 IERC20(campaign.rewardToken).transfer(302 campaign.owner,303 outbound304 );305 }306 // mark campaign as canceled307 rewardCampaigns[campaignId].cancelBlock = lastBlockUpdate;308
309 emit CampaignCanceled(campaignId, halt, outbound, msg.sender);310 return outbound;311}Recommendation:
We advise the code to instead implement a Merkle root per campaign, ensuring that its update can be prevented at the code level by simply ignoring a merkleRoot update that occurs beyond the block that the campaign was cancelled in.
Alternatively, we advise the cancelBlock member to be removed from the RewardCampaign structure and a cancellationMerkleRoot member to be introduced instead which is assigned to the Merkle root active at the time of cancellation, effectively "freezing" all claims to that root's state.
Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):
The Steer Protocol team evaluated this exhibit and specified that they do not consider this a valid concern as they expect the off-chain orchestrators to abide by rigorous operational requirements, ensuring that a campaign's pause will be timely detected and thus not included in the Merkle root update.
We would like to note that the scalability concerns of this exhibit remain, however, we will mark the exhibit as acknowledged based on the fact that the Steer Protocol team has adequately reviewed and tested their off-chain solution to operate within their operational parameters securely.
SRD-07M: Inexistent Increase of Campaign Credit
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | SmartRewardsDistributor.sol:L158 |
Description:
The SmartRewardsDistributor::claim function will improperly handle the campaignCredit as it will not increment it per claim, permitting multiple claims to cumulatively exceed the SmartRewardsDistributor::getAvailableFundsUpToBlock limitation as long as each claim is below it.
Impact:
The campaign-wide limitation is never enforced properly, permitting a campaign to distribute more funds than it is limited to. Additionally, a cancellation of a campaign that has already distributed funds and is on-going will permit the campaign creator to siphon funds from the contract meant for other campaigns.
Example:
136for (uint256 i; i < arrayLength; i++ ) {137 uint256 id = campaignIds[i];138 RewardCampaign memory campaign = rewardCampaigns[id];139 uint256 amount = amounts[i];140 address user = users[i];141
142 // allow owner to withdraw unhalted paused funds143 if (pausedCampaigns[id] && caller != own) {continue;}144
145 bytes32 leaf = keccak256(abi.encodePacked(user, id, amount));146 bool isValidLeaf = MerkleProofUpgradeable.verify(147 proofs[i],148 merkleRoot,149 leaf150 );151 require(isValidLeaf, "Calculated Merkle Hash is invalid");152
153 uint256 outbound = amount - claims[user][id];154 if (outbound == 0) {155 continue;156 }157 // credit campaign & check amount158 uint256 totalCampaignCredit = campaignCredit[id] + outbound;159 require(getAvailableFundsUpToBlock(campaign, lastBlockUpdate) >= totalCampaignCredit, "Campaign allocation exceeds distribution, check if halted!");160 claims[user][id] = amount;161 // Check moved here to presersve root hash and accounting162
163 164 if (caller == own) {165 // allow owner to take custody only if rewards abandoned166 require(block.number >= campaign.abandonmentDeadlineBlock, "Custody not transfered yet");167 user = own;168 }169
170 IERC20(campaign.rewardToken).transfer(user, outbound);171 emit UserClaim(user, campaign.rewardToken, outbound, id, totalCampaignCredit);172}Recommendation:
We advise the code to increase the campaignCredit after the claims update of the user, ensuring that the campaign-wide limitation meant to be imposed by the SmartRewardsDistributor::getAvailableFundsUpToBlock function is imposed correctly.
Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):
The campaign credit is properly updated whenever a SmartRewardsDistributor::claim occurs, ensuring that the relevant limitations are correctly upheld and that the cancellation based exploitation vector is no longer executable.