Stake and withdraw in the same block defeats the purpose of staking and discounts tickets by up to 19.7%

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/staking/Staking.sol#L67-L89 https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/staking/Staking.sol#L118-L124

Vulnerability details

Impact

Stake and withdraw in the same block completly defeats the purpose of staking as anyone can either bundle transaction to sandwich large ticket buy orders or use flashloans to stake, buy tickets and withdraw in a single transaction to essentially receive a ~19% discount on tickets as long as there is more liquidity on DEXes than staked. As staking becomes almost obsolete with this issue, it should always hold that DEX liquidity >> staked liquidity. Rated high, because it impacts both stakers and ticket buyers, is easy to execute and makes a part of the protocol obsolete.

Proof of Concept

Neither the stake and withdraw functions nor the _updateReward function do any kind of check on timestamps or block numbers. This means one can

• stake and withdraw in the same transaction, which allows using flashloans to get a discount on buying tickets yourself
• stake and withdraw in the same block, which allows using a sandwich attacks to take most of the 20% staking rewards from any ticket buy from other users, leaving nothing for permanent stakers.

Technicals

The technicals are the same for both attacks:

1. Buy/Flashloan large amount of stakingToken (LOT).
2. Stake it all, thus becoming a large percentage (P) of the total staking liquidity.
3. Ticket purchase happens.
4. Collect the staking rewards from bought tickets (20% P TotalBuyVolume).
5. Withdraw stakingToken (LOT) and sell/repay it for the same price you bought it (because no other transaction can happen between these steps) only paying the DEXes Buy/Sell/Flashloan fee.

You can add a test case to Staking.t.sol to verify the possibility of staking, buying tickets, claiming rewards and withdrawing in the same block or tx:

// The whole function simulates what could happen in a single block and demonstrates "taking" staking rewards from other stakers
function testGetRewardsMultipleStakersSplitSingleTx() public {
    address staker2 = address(420);
    vm.prank(address(lottery));
    stakingToken.mint(STAKER, 1);
    vm.startPrank(STAKER);
    stakingToken.approve(address(staking), 1);
    staking.stake(1);
    vm.stopPrank();
    vm.prank(address(lottery));
    stakingToken.mint(staker2, 999);
    uint256 preRewardClaimBalanceStaker1 = rewardToken.balanceOf(STAKER);
    uint256 preRewardClaimBalanceStaker2 = rewardToken.balanceOf(staker2);

    // START TX <- The following could be done in a single tx by staker2 for ticket discount
    vm.startPrank(staker2);
    stakingToken.approve(address(staking), 999);
    staking.stake(999);
    buySameTickets(lottery.currentDraw(), uint120(0x0F), address(0), 1000);
    staking.getReward();
    staking.withdraw(999);
    vm.stopPrank();
    // STOP TX

    vm.prank(STAKER);
    staking.getReward();

    assertEq(rewardToken.balanceOf(STAKER) - preRewardClaimBalanceStaker1, TICKET_FEE);
    assertEq(rewardToken.balanceOf(staker2) - preRewardClaimBalanceStaker2, TICKET_FEE * 999); // Staker2 takes almost all of the rewards
}

Tools Used

VSCode, Manual review

Recommended Mitigation Steps

I recommend either updating rewards only once per block (this would save gas, too) or tracking the last staked blocks for every user and preventing withdraw in the same block. For the first solution add this to Staking.sol

    mapping(address => uint256) userLastUpdateTimestamp;

    ...

    function _updateReward(address account) internal {
        if(userLastUpdateTimestamp[account] == block.timestamp) {
            return;
        }
        ...
    }

For the second solution add the following:

    mapping(address => uint256) userLastStakeTimestamp;

    function stake(uint256 amount) external override {
        // _updateReward is not needed here as it's handled by _beforeTokenTransfer
        if (amount == 0) {
            revert ZeroAmountInput();
        }
        userLastStakeTimestamp[msg.sender] = block.timestamp; // <-  Added this

        stakingToken.safeTransferFrom(msg.sender, address(this), amount);
        _mint(msg.sender, amount);

        emit Staked(msg.sender, amount);
    }

    function withdraw(uint256 amount) public override {
        // _updateReward is not needed here as it's handled by _beforeTokenTransfer
        if (amount == 0) {
            revert ZeroAmountInput();
        }
        // Added this if
        if (userLastStakeTimestamp[msg.sender] == block.timestamp) {
            revert("Can't withdraw in the same block as staking");
        }

        _burn(msg.sender, amount);
        stakingToken.safeTransfer(msg.sender, amount);

        emit Withdrawn(msg.sender, amount);
    }

You can replace block.timestamp with block.number

[thereksfour]

Users can make sandwich attacks on lottery purchases ( themselves or others). Consider Med.

[c4-judge]

thereksfour marked the issue as primary issue

[c4-judge]

thereksfour changed the severity to 2 (Med Risk)

[d3e4]

I think the staking system is misunderstood here. It is based on tickets sold, not time staked. The attack vector involves a time/block trick (e.g. flashloan) whereas the attacked system is time/block-independent.

Staking is meant to reward stakers for any ticket purchases that occur during their staking. It depends only on the number of tickets sold, NOT the time staked. It therefore seems absolutely in line with the stated intention to be able to concentrate staking to a specific period (such as one block) of ticket purchases, as long as that is when the ticket purchases happen. LOT has little use besides staking, so deciding to stake only when tickets are purchased and otherwise unstake has no impact on the reward system and is rather pointless.

The possibility to do this with a flashloan might be a consideration for the sponsor. But if LOT ever becomes available for flashloans, then this opportunity will increase the flashloan fees in equal measure through competition, rendering this simply an alternative method of staking.

[c4-judge]

thereksfour marked the issue as duplicate of #126

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)

[c4-judge]

thereksfour marked the issue as grade-c