RewardsPoolFactory Manual Review Findings

RPF-01M: Potential for Lock of Funds

Description:

The extend function of a RewardsPoolBase implementation can result in funds being transferred back to the caller of the function should the extension lead to a reduction in the total rewards paid out and there currently is no way to withdraw these funds from the contract.

Example:

Copy
101function extendRewardPool(
102    uint256 _endBlock,
103    uint256[] memory _rewardsPerBlock,
104    address _rewardsPoolAddress
105) external onlyOwner {
106
107    RewardsPoolBase pool = RewardsPoolBase(_rewardsPoolAddress);
108    uint256 currentEndBlock = pool.endBlock();
109
110    for (uint256 i = 0; i < _rewardsPerBlock.length; i++) {
111        uint256 currentRemainingReward = calculateRewardsAmount(block.number, currentEndBlock, pool.rewardPerBlock(i));
112        uint256 newRemainingReward = calculateRewardsAmount(block.number, _endBlock, _rewardsPerBlock[i]);
113
114        address rewardsToken = RewardsPoolBase(_rewardsPoolAddress).rewardsTokens(i);
115
116        if (newRemainingReward > currentRemainingReward) {
117            // Some more reward needs to be transferred to the rewards pool contract
118            IERC20Detailed(rewardsToken).safeTransfer(_rewardsPoolAddress, newRemainingReward.sub(currentRemainingReward));
119        }
120    }
121
122    RewardsPoolBase(_rewardsPoolAddress).extend(
123        _endBlock,
124        _rewardsPerBlock
125    );
126
127}

Recommendation:

We advise that a withdrawal pattern is coded that enables the extraction of tokens refunded by the extend function either via a seperate function or by introducing statements after the extend invocation of the extendRewardPool function.

Alleviation:

The AbstractPoolsFactory implementation was adjusted to introduce a new withdrawRewardsLeftovers function solely invoke-able by the owner that retrieves any _rewardsToken balance the contract may have and sneds them to the owner.

RPF-02M: Non-Standard Pool Seeding

Description:

The current deploy function implementation expects the funds that are meant to seed a particular pool's deployment to be transferred to the contract ahead of time instead of utilizing the conventional transferFrom pattern.

Example:

Copy
79for (uint256 i = 0; i < _rewardsTokens.length; i++) {
80    uint256 rewardsAmount =
81        calculateRewardsAmount(
82            _startBlock,
83            _endBlock,
84            _rewardPerBlock[i]
85        );
86    IERC20Detailed(_rewardsTokens[i]).safeTransfer(
87        rewardsPoolBase,
88        rewardsAmount
89    );
90}

Recommendation:

We advise that the transferFrom paradigm is implemented instead since it prohibits funds remaining at rest within the contract and is generally considered a better security practice as it disallows the utillization of accidentally sent funds to the factory from being consumed by the pool creation process.

Alleviation:

The changes necessary for this exhibit to be alleviated would require a design change and thus the Alliance Block team opted not to proceed with them.