RewardsPoolBase Manual Review Findings

RPB-01M: Incorrect Rewards & Multiplier Calculations

Description:

The linked SafeMath operation chains conduct either a mul or div operation with the value literal 1e18. As per the original implementation, this number is meant to signify the number 10 to the power of the numebr of decimals the underlying reward token retains. This is not factored in the new implementation thus causing incompatibility and reward calculation deviations with tokens that contain less than 18 decimals such as stablecoins.

Example:

Copy
261for (uint256 i = 0; i < rewardsTokens.length; i++) {
262    uint256 newReward = blocksSinceLastReward.mul(rewardPerBlock[i]); // Get newly accumulated reward
263    uint256 rewardMultiplierIncrease =
264        newReward.mul(1e18).div(totalStaked); // Calculate the multiplier increase
265    accumulatedRewardMultiplier[i] = accumulatedRewardMultiplier[i].add(
266        rewardMultiplierIncrease
267    ); // Add the multiplier increase to the accumulated multiplier
268}

Recommendation:

We recommend that either the literal is replaced by a dynamic evaluation of the token's decimals or that the constructor of the contract ensures each reward token contains only 18 decimals to ensure all calculations within are carried out properly.

Alleviation:

The linked statements were adjusted to first fetch the corresponding decimals from the ERC20 token and adjust the multiplier accordingly.

RPB-02M: Insufficient Siphoning Protection

Description:

The linked code block is meant to allow withdrawing rewards accumulated from different pools for providing liquidity, however, the validation check of the input lpTokenContract only ensures that the rewardsTokens are not withdrawn and does not check against the stakingToken.

Example:

Copy
502function withdrawLPRewards(address recipient, address lpTokenContract)
503    external
504    nonReentrant
505    onlyFactory
506{
507    uint256 currentReward =
508        IERC20Detailed(lpTokenContract).balanceOf(address(this));
509    require(
510        currentReward > 0,
511        "WithdrawLPRewards::There are no rewards from liquidity pools"
512    );
513
514    for (uint256 i = 0; i < rewardsTokens.length; i++) {
515        require(
516            lpTokenContract != rewardsTokens[i],
517            "WithdrawLPRewards::Cannot withdraw from token rewards"
518        );
519    }
520    IERC20Detailed(lpTokenContract).safeTransfer(recipient, currentReward);
521    emit WithdrawLPRewards(currentReward, recipient);
522}

Recommendation:

We advise that a require check is imposed ensuring that the token to withdraw is not the underlying staking token.

Alleviation:

An additional require check was introduced that prevents withdrawing the underlying staking token thus rendering the function safe.