Omniscia Steer Protocol Audit

SmartRewardsDistributor Code Style Findings

SmartRewardsDistributor Code Style Findings

SRD-01C: Combination of Data Structures

Description:

The referenced data structures all utilize a campaignId as a key either directly or in between their resolution.

Example:

contracts/SmartRewardsDistributor.sol
66mapping(uint256 => RewardCampaign) public rewardCampaigns;
67

68// Total spent by a campaign, a check used to track if campaign is over allocating
69mapping(uint256 => uint256) public campaignCredit;
70

71mapping(uint256 => bool) public pausedCampaigns;
72

73address public orchestratorRelay;
74

75/// @dev Mapping of cumulative amount claimed by each address for each campaignId.
76/// user - campaignId - amount
77mapping(address => mapping(uint256 => uint256)) public claims;

Recommendation:

We advise all of them to be combined under the existing RewardCampaign structure, greatly optimizing their gas cost when they are utilized together by minimizing the number of mapping lookups that need to be performed.

Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):

The campaign credit as well as whether it has been paused has been properly introduced to the RewardCampaign structure whilst the claims entry has been retained separate.

As the former two of the exhibit's referenced data structures were considered more relevant to the RewardCampaign structure, we consider this exhibit fully addressed.

SRD-02C: Deprecated Revert Pattern

Description:

The referenced if-revert statements will issue a revert statement with a string literal which has been deprecated.

Example:

contracts/SmartRewardsDistributor.sol
354/// @dev for emergencies or misRegisters, cancel campaign should be used in most circumstances
355function 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 unpaused
361        revert("Cannot unpause canceled campaign!");
362    }
363    pausedCampaigns[campaignId] = state;
364    emit CampaignPaused(campaignId, msg.sender, state);
365    return state;
366}
367

368modifier onlyOrchestrator() {
369    // This function reverts if msg.sender is not a keeper
370    require(msg.sender == orchestratorRelay, "Orchestrator call via dynamic vault only");
371    _;
372}

Recommendation:

We advise custom error declarations to be utilized instead, or the if-revert patterns to be updated to require checks in uniformity with other require checks in the SmartRewardsDistributor contract.

Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):

The referenced if-revert checks have been properly converted to require checks, addressing this exhibit.

SRD-03C: Generic Typographic Mistake

Description:

The referenced line contains a typographical mistake (i.e. private variable without an underscore prefix) or generic documentational error (i.e. copy-paste) that should be corrected.

Example:

contracts/SmartRewardsDistributor.sol
65CountersUpgradeable.Counter public _rewardCampaignIdTracker;

Recommendation:

We advise this to be done so to enhance the legibility of the codebase.

Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):

The underscore prefix has been properly removed from the public variable, addressing this exhibit.

SRD-04C: Ineffectual Usage of Safe Arithmetics

Description:

The linked mathematical operation is guaranteed to be performed safely by surrounding conditionals evaluated in either require checks or if-else constructs.

Example:

contracts/SmartRewardsDistributor.sol
205uint256 campaignFee = steerFee >= discount ? steerFee - discount : 0;

Recommendation:

Given that safe arithmetics are toggled on by default in pragma versions of 0.8.X, we advise the linked statement to be wrapped in an unchecked code block thereby optimizing its execution cost.

Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):

The referenced calculations have all been wrapped in an unchecked code block safely, optimizing the code's gas cost.

SRD-05C: Inefficient mapping Lookups

Description:

The linked statements perform key-based lookup operations on mapping declarations from storage multiple times for the same key redundantly.

Example:

contracts/SmartRewardsDistributor.sol
153uint256 outbound = amount - claims[user][id];
154if (outbound == 0) {
155    continue;
156}
157// credit campaign & check amount
158uint256 totalCampaignCredit = campaignCredit[id] + outbound;
159require(getAvailableFundsUpToBlock(campaign, lastBlockUpdate) >= totalCampaignCredit, "Campaign allocation exceeds distribution, check if halted!");
160claims[user][id] = amount;

Recommendation:

As the lookups internally perform an expensive keccak256 operation, we advise the lookups to be cached wherever possible to a single local declaration that either holds the value of the mapping in case of primitive types or holds a storage pointer to the struct contained.

As the compiler's optimizations may take care of these caching operations automatically at-times, we advise the optimization to be selectively applied, tested, and then fully adopted to ensure that the proposed caching model indeed leads to a reduction in gas costs.

Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):

The latter of the two pairs has been optimized, however, the former remains suboptimal.

To note, the recommendation of this exhibit is to store the claims[user] lookup to a local mapping(uint256 => uint256) storage variable which is permitted and would significantly optimize the loop's gas cost.

SRD-06C: Inexistent Error Message

Description:

The linked require check has no error message explicitly defined.

Example:

contracts/SmartRewardsDistributor.sol
326require(newOwner != address(0));

Recommendation:

We advise one to be set so to increase the legibility of the codebase and aid in validating the require check's condition.

Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):

A descriptive error message has been introduced for the referenced require check, addressing this exhibit.

SRD-07C: Misleading Maximum Fee

Description:

The underscore separator as utilized infers that the maximum fee would be 10%, however, the maximum steer fee is 100% which we consider incorrect.

Example:

contracts/SmartRewardsDistributor.sol
52uint256 internal constant MAX_STEER_FEE = 10_000;
53uint256 internal constant FEE_DIVISOR = 100_00;

Recommendation:

We advise one trailing zero to be removed from the MAX_STEER_FEE variable, properly enforcing a maximum limitation of 10%.

Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):

The maximum fee variable has been updated from 10_000 (being equivalent to 100%) to 25_00 (being equivalent to 25%), ensuring that it illustrates the limitation correctly.

SRD-08C: Optimization of Condition

Description:

The SmartRewardsDistributor::claim function will repeatedly evaluate whether the caller is the OwnableUpgradeable::owner of the contract redundantly.

Example:

contracts/SmartRewardsDistributor.sol
133address caller = msg.sender;
134address own = owner();
135

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 funds
143    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        leaf
150    );
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 amount
158    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 accounting
162

163    
164    if (caller == own) {
165        // allow owner to take custody only if rewards abandoned
166        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 this comparison to be performed once and stored as a bool that is consequently reused within the for loop, optimizing its gas cost.

Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):

The msg.sender == owner() conditional is cached outside the for loop, optimizing its usage and thus the function's gas cost.

SRD-09C: Redundant Parenthesis Statement

Description:

The referenced statement is redundantly wrapped in parenthesis (()).

Example:

contracts/SmartRewardsDistributor.sol
389(keccak256(lastChar) != keccak256(bytes("O")) &&
390    keccak256(lastChar) != keccak256(bytes("I")) &&
391    keccak256(lastChar) != keccak256(bytes("l")));

Recommendation:

We advise them to be safely omitted, increasing the legibility of the codebase.

Alleviation (e31556397b9d321737ff47809b572383dda4a2aa):

The redundant parenthesis in the referenced statement have been safely omitted.

Prev

IntegralMultiPositionLiquidityManager.sol (IMP-C)

Next

Finding Types

On this page