Fixed and Dynamic Staking

Proof of concept: PoC

FixedStaking can be deployed by anyone using any two tokens as stakeToken and rewardToken. The earned amount of rewards are calculated in calculateRewardAmountFutureDate() as

FixedPointMathLib.mulDiv(stakeAmount, (scaledInterestRate - WAD), WAD)

This means the result ends up being in stake token decimals even though it's used in the context of rewards. This mistake leads to unexpected results depending on what decimals the tokens have and how many rewards are currently available.

For example, if the stakeToken has 18 decimals, but the rewardToken has 6, in most cases the earned amount will be stored in pendingRewards and users won't be able to claim it or if they do, they will earn a lot more than they have to.

The other case is also possible - 6 decimals for stakeToken and 18 decimals for rewardToken - then users will receive almost nothing compared to what they are entitled to.

Store the two tokens' decimals by loading them in the constructor and modify calculateRewardAmountFutureDate() to return the result in rewards decimals.

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

Users can initiate last minute deposits prior to the admin updating the stake state. The user is able to enter a large lockDuration .

They will immediately be able to claim the rewards when the stake state changes to SKIP_LOCKUP_ALLOWED and the large lock duration will apply the maxBoost to the user’s rewards, even though their funds were only locked up for a short time.

Ensure that the unlock time can not be later than the compoundEndDate to avoid last second locks.

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

Proof of concept: PoC

FixedStaking.calculateInterestPercentageFutureDate() calculates elapsedTime by taking the maximum between (0, currentTimestamp - startDate).

The problem is that both currentTimestamp and startDate are unsigned integers. Because of this, when startDate > currentTimestamp, the result won't be 0, but the transaction with revert with an underflow instead.

This can happen when a user has staked after compoundingEndDate. The user will lose their staked funds as a result.

• uint256 elapsedTime = FixedPointMathLib.max(0, currentTimestamp - startDate);
+ uint256 elapsedTime = currentTimestamp > startDate • currentTimestamp -
startDate :  0;

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

DynamicStaking.unstake() and DynamicStaking.claim() require that msg.value = unstakeFee and msg.value = claimFee respectively.

However, the Multicall contract is inherited which allows users to execute multiple unstake() and claim() calls in the same transaction.

As a result, the msg.value will always be the same value for the different calls and users can execute the function many times, but pay only once.

If claimFee = unstakeFee, they can even batch a call to both claim() and unstake() and pay only once.

An easy solution is to add a check to the constructor that if the fees are positive, they must not be equal. However, this solution limits the flexibility of the contract.

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

FixedStaking.getClaimable() returns the amount of rewards earned by a given active stake or all active stakes.

However, it doesn't check if the state is FORCEFULLY_TERMINATED. If it is, the actual claimable amount is 0, but they will return a positive amount.

Return 0 if state = State.FORCEFULLY_TERMINATED.

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

The constructor of FixedStaking ensures that poolCapacity > 0, but this check is not present in the changePoolCapacity() function.

Calling that function right after deployment bypassed the check in the constructor and the value can be set to 0.

Consider implementing the check in changePoolCapacity() as well.

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

FixedStaking.stake() allows staking on behalf of any user without any restrictions on the staked amount besides it's positive.

This allows anyone to spam the stakes of another user by depositing 1 wei multiple times and in result because getClaimable() and getClaimableIncludingPending() revert with OOG when looping through the stakes. It can also cause problems for any integrators using getNumberOfStakes().

Consider requiring minimum stake amount if initiator = onBehalfOf

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

The changePoolCapacity() function in FixedStaking allows the admin to change the capacity of the pool at any given point in time.

They can use it to change the capacity to a lower value, but anyone can frontrun their transaction and stake before the changes have taken place.

For example:

• totalStaked = 1000
• poolCapacity = 1500
• the admin submits a transaction to change poolCapacity to 1200
• A staker frontruns them (or their transaction just ends up earlier in the block) and stakes up to 1500
• poolCapacity is set to 1200, but totalStaked = 1500

This is a common problem encountered with ERC20.approve() as well. You can document this behavior and be careful when you change the limit.

Magna Team: Acknowledged.

The function withdrawOnBehalf() is marked as payable but does not require a fee for its execution.

Consider removing the payable keyword to avoid confusion.

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

FixedStaking.stake() incorrectly reverts with the ZeroAddressPassed() error if the stake is using a lock less than the minimum allowed one.

It should revert with MinimumLockDurationViolated() instead.

• require(lockDuration = minimumLockDuration, ZeroAddressPassed());
+ require(lockDuration = minimumLockDuration, MinimumLockDurationViolated());

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

Once DynamicStaking.defundContractBalance() is called, all of the tokens in the contract - stakes + unstakes + rewards are send to a recipient address, but totalStaked and entries of the user stakes are not modified at all, which breaks the accounting mechanism of the contract.

Consider pausing the contract when defunding so users won't interact with it anymore.

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

DynamicStaking.modifyCampaign() doesn't check the length of the reward period - its end must only be in the future.

If the reward period is too long, it can cause the reward rate to become too low, resulting in 0 rewards for stakers because of rounding issues.

Consider checking that reward rate is at least greater than 0 at the end of modifyCampaign.

require(rewardRemaining * WAD / (periodFinish - lastUpdateTime) > 0);

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

DynamicStaking.modifyCampaign() transfers the token unconditionally in the else statement, even if the amount is 0. This may result in reverts for some tokens that fail on 0 transfer.

It may be a desired use case to call modfyCampaign() with the same amount as the contract holds if for example it couldn't be claimed during the last distribution.

Consider transferring the tokens only if amount is not 0.

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

The SUPER_ADMIN role is the only role allowed to handle the following processes:

Super admin manage other roles like admin can initiate all state transitions
can rescue reward tokens at any time if it was allowed during the creation of
the pool can rescue staked tokens if it was allowed during the creation of the
pool

However, the constructor only checks that there is at least 1 admin role or 1 super admin role, but does not enforce that there is at least 1 super admin role. This will disallow any of the above processes from happening.

require((params.superAdmins.length + params.admins.length > 0)

Ensure that there is at least one super admin role assigned in the constructor.

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

FixedStaking.calculateInterestPercentageFutureDate() computes the interest percentage the user is eligible to receive when they unstake their tokens. This percentage depends on how many periods have passed

periodsElapsed = (elapsedTime) / compoundingPeriodLength

Because there is no scaling applied to these values, unstaking even a second earlier will result in a loss of interest for the user for 1 entire period.

In addition, if the state was transition to TERMINATED or the block.timestamp is beyond compoundingEndDate, then lastValidTimestamp will be used to calculate timeElapsed and the user won't be able to time their unstake transaction even if they are aware of the issue.

The contract configuration is permissionless, this means a period can be 7 days, a year, 4 years, etc... Losing 1 whole period of rewards is a substantial amount in these cases.

function calculateInterestPercentageFutureDate(uint256 startDate, uint256 futureDate)
     public view virtual returns (uint256 interestRate) {uint256 lastValidTimestamp =
terminationTimestamp = 0 • FixedPointMathLib.min(futureDate, terminationTimestamp) :
futureDate; uint256 currentTimestamp = FixedPointMathLib.min(lastValidTimestamp,
compoundingEndDate); uint256 elapsedTime = FixedPointMathLib.max(0, currentTimestamp -
startDate); • uint256 periodsElapsed = elapsedTime / compoundingPeriodLength; + uint256
periodsElapsed = elapsedTime * WAD / compoundingPeriodLength; • uint256 allowedPeriodsElapsed
= maxCompoundingPeriods = 0 • FixedPointMathLib.min(periodsElapsed, maxCompoundingPeriods) :
periodsElapsed; + uint256 allowedPeriodsElapsed = maxCompoundingPeriods = 0 •
FixedPointMathLib.min(periodsElapsed, maxCompoundingPeriods * WAD) : periodsElapsed; • int256
interest = FixedPointMathLib.powWad(int256(WAD + interestRatePerPeriod),
int256(allowedPeriodsElapsed * WAD)); + int256 interest = FixedPointMathLib.powWad(int256(WAD
+ interestRatePerPeriod), int256(allowedPeriodsElapsed));
interestRate = uint256(interest);}

Magna Team: Acknowledged.

A lot of the fields passed in IFixedParams.ConstructorParams are not sufficiently validated:

• compoundingPeriodLength is not capped to a reasonable value. If it's set to a very large value like

type(uint256).max, every time periodElapsed which is calculated as elapsedTime / compoundingPeriodLength will end up being 0 and users will never receive rewards.

• interestRatePerPeriod should be a value in a reasonable range. If it's not it's possible to grief users

by exploiting WAD + interestRatePerPeriod > type(uint256).max and making calculateInterestPercentageFutureDate() always revert. It should also be in reasonable ranges because if it's too big, there will never be enough rewards to be paid out.

• withdrawRewardsDeadline must not be lower than a given value chosen by the protocol because

now it can be set to 0 which allows the admin to transition the state from INITIALIZED all the way to REWARD_RESCUABLE in a single block and withdraw all the rewards.

• boostYOffset and boostdYdX should also be in reasonable ranges because boostYOffset +

boostdYdX * extraLockSeconds should not surpass type(uint256).max

Implement the proposed validations in the FixedStaking constructor.

Magna Team: Acknowledged.

In the emergencyRescueStakeTokens function, there is the following check:

if (tokenAmountToRescue > 0)
{totalStaked = rescuedTokenAmount; stakeToken.safeTransfer(rescuedTokenReceiver,
rescuedTokenAmount);}

However, this should always evaluate to true because tokenAmountToRescue is a function parameter. It would not make sense to call this function with a zero value.

Use rescuedTokenAmount in the if clause instead.

Magna Team: Resolved in commit 74b3f78a8917a425b0893a5ad09d5ef93d158d1b.

DynamicStaking.withdrawOnBehalf() indicates that users are able to withdraw on behalf of another user, however, the function always withdraws from msg.sender.

Consider renaming this function to withdraw() to follow similar conventions in the rest of the codebase.

Magna Team: Resolved.

The unstakeAndClaim() function allow users to receive their rewards to another address, however, claimPendingRewards() does not. The rewards are always transferred to msg.sender.

Consider adding a parameter for the user to specify which address should receive the rewards.

Magna Team: Resolved.

Users are able to create locks of any duration larger than the minimum lock duration. This may cause issues if a user initiates a deposit with an enormously large lock duration by accident.

Impose a strict max lock duration, e.g. 4 years.

Magna Team: Resolved.