code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/Lottery.sol#L110-L115

Vulnerability details

Impact

Player can get frontend rewards if they pass in their own address in buyTicket() or DoS the frontend rewards by filling in any random frontend address.

Proof of Concept

When a player buys a ticket, he calls buyTickets() and passes in the address of the frontend. Because the frontend address is not checked, the player can set the frontend address to be himself.

    function buyTickets(
        uint128[] calldata drawIds,
        uint120[] calldata tickets,
        @--audit frontend can be set by the player, can set any address
        address frontend,
        address referrer
    )

When the time comes to claimRewards(), the beneficiary can be the player's address.

    function claimRewards(LotteryRewardType rewardType) external override returns (uint256 claimedAmount) {
        address beneficiary = (rewardType == LotteryRewardType.FRONTEND) ? msg.sender : stakingRewardRecipient;
        claimedAmount = LotteryMath.calculateRewards(ticketPrice, dueTicketsSoldAndReset(beneficiary), rewardType);

        emit ClaimedRewards(beneficiary, claimedAmount, rewardType);
        rewardToken.safeTransfer(beneficiary, claimedAmount);
    }

The contract then calls dueTicketsSoldAndReset, gets the dueTickets and resets the frontendDueTicketSales[beneficiary]. However, since the address of the beneficiary is the player, the player can just claim the frontend rewards for himself.

    function dueTicketsSoldAndReset(address beneficiary) private returns (uint256 dueTickets) {
        if (beneficiary == stakingRewardRecipient) {
            dueTickets = nextTicketId - claimedStakingRewardAtTicketId;
            claimedStakingRewardAtTicketId = nextTicketId;
        } else {
            dueTickets = frontendDueTicketSales[beneficiary];
            frontendDueTicketSales[beneficiary] = 0;
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend making the frontend address changeable only through the owner or set in the constructor like how stakingRewardRecipient is. Otherwise, have a list of frontends that can receive rewards. Make sure the frontend address is already set and not set by the player.

c4-judge commented 1 year ago

thereksfour marked the issue as duplicate of #483