【Tomo-H#2】Stolen winnings by Front-running

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/interfaces/ILottery.sol#L154-L159 https://github.com/code-423n4/2023-03-wenwin/blob/main/src/Lottery.sol#L170-L176

Vulnerability details

Impact

Even if users win the Lottery, the attacker can steal the winnings by front-running attack.

Proof of Concept

/// ILottery.sol

/// @dev Transfer all winnings to `msg.sender` for the winning tickets.
/// It reverts in case of non winning ticket.
/// Only ticket owner can claim win, if any of the tickets is not owned by `msg.sender` it will revert.
/// @param ticketIds List of ids of the tickets being claimed.
/// @return claimedAmount Amount of reward tokens claimed to `msg.sender`.
function claimWinningTickets(uint256[] calldata ticketIds) external returns (uint256 claimedAmount);

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/interfaces/ILottery.sol#L154-L159

/// Lottery.sol
function claimWinningTickets(uint256[] calldata ticketIds) external override returns (uint256 claimedAmount) {
    uint256 totalTickets = ticketIds.length;
    for (uint256 i = 0; i < totalTickets; ++i) {
        claimedAmount += claimWinningTicket(ticketIds[i]);
    }
    rewardToken.safeTransfer(msg.sender, claimedAmount);
}

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/Lottery.sol#L170-L176

The claimWinningTickets() transfer all winnings to msg.sender for the tickets.

In this function, there is no checking the buyer of tickets are equal to the msg.sender

And only the ticketId depends on whether that ticketId is a winning ticket.

Therefore, the following can happen

1. Alice buy tickets.
2. Alice won the Lottery and executed claimWinningTickets() to get paid a reward.
3. Eve checks the arguments of the transaction executed by Alice and performs this function with the same ideas at a higher gas fee than Alice
4. Finally, Eve would get the winnings instead of Alice.

Tools Used

Manual

Recommended Mitigation Steps

Check that the ticket buyer is equal to the msg.sender in executing claimWinningTickets() to prevent a front-running attack.

[thereksfour]

The check already exists

    function claimWinningTicket(uint256 ticketId) private onlyTicketOwner(ticketId) returns (uint256 claimedAmount) {
...
    modifier onlyTicketOwner(uint256 ticketId) {
        if (ownerOf(ticketId) != msg.sender) {
            revert UnauthorizedClaim(ticketId, msg.sender);
        }
        _;
    }

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid