Steal Rewards by FrontRunning

[code423n4]

Lines of code

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

Vulnerability details

Impact

Attacker can perform frontrunning attack in order to get reward without staking any token. This happens by correctly positioning the staking and withdraw operation with buyTickets operation

Proof of Concept

1. User A is calling buyTickets function to buy 5 tickets

2. Attacker see this operation and instantly call below 2 transactions:

a. The first operation with high gas is for staking attacker funds b. The second operation with low gas is for getReward and withdraw attacker funds

3. So, first Attacker stake operation triggers with say amount 10000

function stake(uint256 amount) external override {
        // _updateReward is not needed here as it's handled by _beforeTokenTransfer
        if (amount == 0) {
            revert ZeroAmountInput();
        }

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

        emit Staked(msg.sender, amount);
    }

4. Now User A, buyTickets executes which buys say 20 tickets

...
ticketId = mint(msg.sender, drawId, ticket);
        unclaimedCount[drawId][ticket]++;
        ticketsSold[drawId]++;
        emit NewTicket(currentDraw, ticketId, drawId, msg.sender, ticket, frontend, referrer);
...

5. Attacker getReward function executes which internally calls _updateReward

function _updateReward(address account) internal {
        uint256 currentRewardPerToken = rewardPerToken();
        rewardPerTokenStored = currentRewardPerToken;
        lastUpdateTicketId = lottery.nextTicketId();
        rewards[account] = earned(account);
        userRewardPerTokenPaid[account] = currentRewardPerToken;
    }

6. rewardPerToken increases as 20 more tickets have been sold from last time

uint256 ticketsSoldSinceUpdate = lottery.nextTicketId() - lastUpdateTicketId;
        uint256 unclaimedRewards =
            LotteryMath.calculateRewards(lottery.ticketPrice(), ticketsSoldSinceUpdate, LotteryRewardType.STAKING);

        return rewardPerTokenStored + (unclaimedRewards * 1e18 / _totalSupply);

7. This means User earns rewards on this increased rewardPerToken for the full staked amount which is 10000

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

8. Finally withdraw function is executed which withdraws the User staked fund

9. Notice User has earned reward even though he has staked and withdrawn in the same block so practically no staking at all

Recommended Mitigation Steps

Staking rewards should also depends on the time uptil which user has staked the amount.

[c4-judge]

thereksfour marked the issue as duplicate of #479

[c4-judge]

thereksfour marked the issue as not a duplicate

[c4-judge]

thereksfour marked the issue as primary issue

[rand0c0des]

the size of the buying ticket needs to be large, as well as portion of LOT amount in order for this to even make sense economically. We designed staking in a this way and we do not consider this as an issue.

[c4-sponsor]

rand0c0des marked the issue as sponsor disputed

[thereksfour]

Considering that it has been clearly mentioned in the documentation, invalid

There is no locking, no ramp-up, no staking fees, no withdrawal penalties, and no additional attempts to obfuscate DAI cash flow from ticket sales. LOT holders can stake or unstake at any time without penalty.

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)

[c4-judge]

thereksfour marked the issue as grade-c