Misconfigured pool can lead to rewards locked indefinitely

Summary

Due to weak validation of the BasePool initialization function (__BasePool_init) it's possible to deploy a pool with a misconfigured escrow pool: escrowPool can be set to the zero address while escrowPortion is set to a valid value. In such a pool, depositors will get only a portion of reward tokens (as per escrowPortion) and the remaining part will remain locked in the pool and unclaimable indefinitely.

Vulnerability Detail

The root cause is in these lines:

function claimRewards(address _receiver) external {
    uint256 rewardAmount = _prepareCollect(_msgSender());
    // @audit escrow amount is extracted before escrow pool configuration is validated
    uint256 escrowedRewardAmount = rewardAmount * escrowPortion / 1e18;
    uint256 nonEscrowedRewardAmount = rewardAmount - escrowedRewardAmount;

    if(escrowedRewardAmount != 0 && address(escrowPool) != address(0)) {
        escrowPool.deposit(escrowedRewardAmount, escrowDuration, _receiver);
    }

    // ignore dust
    if(nonEscrowedRewardAmount > 1) {
        rewardToken.safeTransfer(_receiver, nonEscrowedRewardAmount);
    }
    ...
}

Here, rewardAmount is split into escrowedRewardAmount and nonEscrowedRewardAmount before the check for configured escrow pool is made. In case escrowedRewardAmount is non-zero and escrowPool is the zero address:

escrowedRewardAmount will be subtracted from rewardAmount;
escrowedRewardAmount won't be sent to escrowPool because it's not configured;
the user will get only nonEscrowedRewardAmount;

escrowedRewardAmount will remain unclaimable.

Impact

Reward tokens that are expected to be distributed among users pro-rata get locked in a misconfigured pool indefinitely.

Code Snippet

// test/BasePool.ts
it("locks funds when escrow is misconfigured [audit]", async () => {
const testBasePoolFactory = new TestBasePool__factory(deployer);

const DISTRIBUTION_AMOUNT = parseEther("1");
const MINT_AMOUNT = parseEther("10");

// Deploying a misconfigured pool...
const basePool = (await testBasePoolFactory.deploy(
    TOKEN_NAME,
    TOKEN_SYMBOL,
    depositToken.address,
    rewardToken.address,
    constants.AddressZero, // escrowPool is not set
    constants.WeiPerEther.div(2), // escrowPortion is set to 50%
    ESCROW_DURATION
)).connect(account1);

await rewardToken.approve(basePool.address, constants.MaxUint256);

// account3 is the only depositor, thus it's eligible for the entire DISTRIBUTION_AMOUNT.
await basePool.mint(account1.address, MINT_AMOUNT);
await basePool.distributeRewards(DISTRIBUTION_AMOUNT);
await basePool.claimRewards(account3.address);

// account3 receives only half of the distribution amount.
expect(await rewardToken.balanceOf(account3.address)).to.eq(DISTRIBUTION_AMOUNT.div(2));
// account3 cannot claim more tokens.
expect(await basePool.withdrawableRewardsOf(account3.address)).to.eq(0);
// The pool keeps holding the remaining distribution amount indefinitely, and it's not claimable by anyone.
expect(await rewardToken.balanceOf(basePool.address)).to.eq(DISTRIBUTION_AMOUNT.div(2));
// escrowPool gets nothing because it's not configured.
expect(await escrowPool.getTotalDeposit(account3.address)).to.eq(0);
});

Tool used

Manual Review

Recommendation

Consider extracting escrow reward amount only if the escrow pool is configured properly (both escrowPool and escrowPortion are set to valid values).

Duplicate of #107

sherlock-audit / 2022-10-merit-circle-judging

Jeiwan - Misconfigured pool can lead to rewards locked indefinitely #56