MultiRewards

Proof of concept: PoC

In the rewards system, rewards are not locked and can be claimed at any point in time, even if the user's staking tokens are locked for the 13 week period.

A user with a large portion of the totalSupply may continuously claim their rewards and lock those tokens to gain an even greater portion, whether it be directly staking if the reward token matches the staking token, or with a swap to the staking token from the reward token.

This will ultimately dilute the rewards for other users in the reward period while massively swaying the rewards towards themselves.

The change in distribution is at the expense of other staked users, as there is a set amount to be distributed and the final rewards are dramatically skewed towards the user due to their continuous compounding while other staked users have their rewards fractionalized.

Consider if this is the expected behavior. If this is expected then clearly document it for users so they have a chance to collect an appropriate amount of rewards. If this is not expected, consider requiring that rewards can only be claimed at the end of the lock period.

Abracadabra Team: The issue was resolved in commit 3081d38.

Guardian Team: Because reward periods do not necessarily align with epochs, rewards may still be compounded within a reward period. Consider forcing reward notifications to adhere exactly to epoch time windows, otherwise be aware of this behavior and clearly document it for users.

Abracadabra Team: Reward periods are now tied directly to epochs in commit 78b7cfa.

In the _updateRewards function the rewardTokens are iterated over to _updateRewardsGlobal for each token. However there is no explicit bound on the length of the rewardTokens array.

As a result it is possible for the rewardTokens array to become so long that updating the rewards for each token requires more gas than the block gas limit allows.

In such a scenario the _updateRewards function and all functions relying on it would be DoS’d. Similarly in the _getRewards function all rewardTokens are iterated over to pay the user’s rewards.

Therefore the owner may DoS the claiming of rewards by adding many rewardTokens with the addReward function.

Consider adding a limit to the amount of rewardTokens that may be supported in the addReward function.

Abracadabra Team: The issue was resolved in commit 0bdf6d5.

In the _rewardPerToken function if the totalSupply is 0 the rewardPerTokenStored is not advanced, meaning that rewards are not accrued until a user stakes.

This behavior is fine when a user does stake at some point during the reward period, however in the case that no users stake for an entire reward period, the rewards for that period will not be distributed and will have to be claimed using the recover function.

In the case where MIM is used as a rewardToken, the undistributed rewards will not be recoverable as the stakingToken cannot be recovered. Therefore any MIM rewards that go undistributed will be locked.

If the totalSupply is 0 before the end of a reward period be sure to stake a trivial amount in order to capture any undistributed MIM rewards before calling the notifyRewardAmount function.

Abracadabra Team: The issue was resolved in commit 710f511.

Guardian Team: Upon recovery the reward rate is not updated to reflect the new reward token balance in the staking contract. Consider adjusting the reward rate, otherwise be careful to not remove tokens that are necessary for user rewards.

In the processExpiredLocks function there is no requirement that the operator must process the oldest expired lock first. Therefore if an operator neglects to process expired locks for a user directly when they unlock it becomes possible for an operator to errantly or maliciously process the lock at the lastLockIndex before processing the other locks.

Depending on where the lastLockIndex is relative to the user’s locks array this can yield different outcomes:

In the following example lock A expired before lock B and lock B expired before lock C, lock C is the true last lock:

• lastLockIndex is 2
• The user’s locks array is [A, B, C]
• All locks are expired, the operator processes lock C first
• The user’s locks array is now [A, B], the lastLockIndex is still 2

However, 2 is an invalid index for the array and now the user cannot create a new lock as the _createLock function will revert with an array index out of bounds error.

Consider requiring that the lock at the lastLockIndex may only be processed when there is only 1 item left in the locks array. Additionally, be sure to carefully process the correct locks on time.

Abracadabra Team: The issue was resolved in commit a312ff3.

In the processExpiredLocks function when the lock being processed is the final lock in the array, the LogLockIndexChanged event will still emit with a fromIndex of the last index and a toIndex of the last index.

In this case no lock index changed, therefore the LogLockIndexChanged event may be misleading.

Consider only emitting the LogLockIndexChanged event when the index != lastIndex.

Abracadabra Team: The issue was resolved in commit 00cc23a.

In the addReward function there is no validation that the rewardToken is not already present in the rewardTokens list. There is no significant issue with a duplicate rewardToken, however it would cause unnecessary gas expenditure upon updating and claiming rewards.

Consider using an EnumerableSet for the rewardTokens and validating that the rewardToken is not already present in the set before adding it in the addReward function.

Abracadabra Team: The issue was resolved in commit bcd8d9a.

The function notifyRewardAmount is used by operators to update the distribution rate and period of a reward token.

The issue is that there is another function addReward which actually adds the reward token to the rewardTokens array state variable.

If the operator uses notifyRewardAmount to distribute a reward token that does not exist in the array, these rewards will not be accounted for as most reward-related functions will loop through the reward token arrays.

Consider adding a check to notifyRewardAmount to verify that the reward token was already added by the owner.

Abracadabra Team: The issue was resolved in commit 1c20437.

In the constructor the maxLocks is assigned to the result of _lockDuration / _rewardsDuration, this result is valid when the _lockDuration is an exact multiple of the _rewardsDuration, but in any case where the _lockDuration is not an exact multiple of the _rewardsDuration the maxLocks is 1 less than it should be due to truncation.

For example:

• _lockDuration = 5 weeks
• _rewardsDuration = 2 weeks
• maxLocks = 5 / 2 = 2
• However 3 distinct reward periods will intersect the 5 week lock period, therefore a user may have a

maximum of 3 locks.

It is unlikely that the _lockDuration would not be a perfect multiple of the _rewardsDuration, however this edge case ought to be either handled or validated against in the constructor.

Either validate that _lockDuration % _rewardsDuration == 0 or assign maxLocks to (_lockDuration + _rewardsDuration • 1) / _rewardsDuration.

Abracadabra Team: The issue was resolved in commit b84ba2f.

The documentation for the withdraw function suggests that the function will “iterate through the locks to find expired locks, prunning them and cumulate the amounts to withdraw”, however the withdraw function does not implement this behavior.

Consider implementing this behavior for the withdraw function or remove the comment on this behavior.

Abracadabra Team: The issue was resolved in commit 1d99a3d.

In the notifyRewardAmount function the rewardRate is assigned to the result of amount / rewardsDuration, which will include precision loss up to 604800 when the rewardsDuration is 1 week long.

This amount of precision loss will be negligible for tokens with 18 decimals, however if a token such as USDC, with 6 decimals, is used as the rewardToken then this amount of precision loss may compound to be nontrivial over time.

Additionally, a malicious operator could leverage this precision loss to cause significant loss of assets for users (if a 6 decimal token is used). The operator could donate a 0 amount to the notifyRewardAmount function and cause reward periods to end when they have only just begun in order to update the rewardRate and cause precision loss on the total amount to be distributed.

The malicious operator could donate a 0 amount in a loop this way to ultimately cause a significant reduction in the rewardRate due to precision loss.

There is already a recover function to rescue any funds lost due to precision. To address the potential gaming by a malicious operator, consider requiring that a non-trivial amount of the token is donated or that the previous period is fully finished before starting a new one.

Abracadabra Team: The issue was resolved in commit bcd8d9a.

The purpose of the processExpiredLocks function is to transfer the locked Balance to unlocked Balance.

The rewards accrued by the user is calculated based on the balanceOf(user), which is calculated in the following way: bal.unlocked + ((bal.locked * lockingBoostMultiplerInBips) / BIPS).

If there is a delay in calling processExpiredLocks then the users will still accrue the rewards for their expired locks.

Consider if this is the expected behavior, otherwise be sure to process expired locks on time.

Abracadabra Team: Acknowledged.

In the constructor there is no validation that the _lockDuration and _rewardsDuration are not too short nor too long.

Additionally there is no validation that the lockingBoostMultiplier is greater than the minimum basis points divisor, otherwise users would be punished for locking rather than being rewarded.

Consider adding validation in the constructor for the _lockDuration, _rewardsDuration, and lockingBoostMultiplier

Abracadabra Team: The issue was resolved in commit a8f386a.

Guardian Team: The MIN_BOOST_MULTIPLIER validation does not ensure locking provides a boost. Consider reverting if _lockingBoostMultiplierInBips <= BIPS instead to ensure there is a boost for locking.

Abracadabra Team: The validation was updated in commit c34decd.

User’s locked balances are updated immediately upon locking, even though technically their locking period does not begin until the end of the week they locked within.

This means that user’s will begin receiving the locking rewards outside of the 13 week locking period, which can reduce the rewards earned by current lockers and may be unexpected by the protocol.

Consider if this is expected behavior, if it is not then only allow users to accrue locking rewards once their 13 week lock period has begun.

Abracadabra Team: Acknowledged.

The operator must pass the lockIndexes in consideration of an element’s placement after the length decrements from the prior indexes.

Consider the following scenario:

• Bob has 3 expired locks [0, 1, 2]
• Operator passes lockIndexes = [0, 1, 2]
• After processing indexes 0 and 1, the lock array’s length will only be 1. However, the current index to

process is 2.

• Consequently, processExpiredLocks reverts if (locks[index].unlockTime > block.timestamp) due to

out-of-bounds.

Consider documenting this scenario and ensure operators are able to track the to-be indexes if needed.

Abracadabra Team: We will ensure our gelato task processing locks does this correctly.

When the Arbitrum sequencer is down, transactions to the LockingMultiRewards contract can still be recorded using the DelayedInbox.

This allows users to access the contract but will not allow operators and admin to call the restricted functions through DelayedInbox due to the way address aliasing works.

From the Arbitrum Docs, "when these messages are executed on L2, the sender's address —i.e., that which is returned by msg.sender — will not simply be the L1 address that sent the message; rather it will be the address's "L2 Alias." An address's L2 alias is its value increased by the hex value 0x1111000000000000000000000000000000001111"

L2_Alias = L1_Contract_Address + 0x1111000000000000000000000000000000001111

As a result the owner will not be able to pause or unpause the contract when the sequencer is down, and operators will not be able to process locks nor notify rewards.

Be aware of this scenario and have a contingency plan in the event that the sequencer is down for an extended period.

Abracadabra Team: Acknowledged.