LiquidityMiningCampaignFactory Manual Review Findings

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

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.

LMF-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
82for (uint256 i = 0; i < _rewardsTokens.length; i++) {
83    uint256 rewardsAmount =
84        calculateRewardsAmount(
85            _startBlock,
86            _endBlock,
87            _rewardPerBlock[i]
88        );
89    IERC20Detailed(_rewardsTokens[i]).safeTransfer(
90        rewardsPoolBase,
91        rewardsAmount
92    );
93}

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.