code-423n4 / 2023-03-wenwin-findings

1 stars 1 forks source link

Protocol doesn't handle fee on transfer token when used as a rewardToken #268

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

When fee on transfer token is used as a reward token then player can purchase large number of tickets for which protocol will receive less amount than ticketPrice * tickets.length, as while transfering funds from player(msg.sender) to Lottery contract, fee will be deducted.

Due to this, if fee on transfer token is charging more fee then there will be a possibility of having less amount in prize pot to be distributed among the ticket winners.

Proof of Concept

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

function buyTickets(
    uint128[] calldata drawIds,
    uint120[] calldata tickets,
    address frontend,
    address referrer
)
    external
    override
    requireJackpotInitialized
    returns (uint256[] memory ticketIds)
{
    if (drawIds.length != tickets.length) {
        revert DrawsAndTicketsLenMismatch(drawIds.length, tickets.length);
    }
    ticketIds = new uint256[](tickets.length);
    for (uint256 i = 0; i < drawIds.length; ++i) {
        ticketIds[i] = registerTicket(drawIds[i], tickets[i], frontend, referrer);
    }
    referralRegisterTickets(currentDraw, referrer, msg.sender, tickets.length);
    frontendDueTicketSales[frontend] += tickets.length;
    rewardToken.safeTransferFrom(msg.sender, address(this), ticketPrice * tickets.length); //@audit-issue
}

Whenever player(msg.sender) tries to buy a ticket, player's ticketPrice * tickets.length rewardTokens are transfered to Lotterey contract but since rewardToken could be a fee on transfer token, Lottery contract will recieve less funds.

rewardToken.safeTransferFrom(msg.sender, address(this), ticketPrice * tickets.length); //@audit-issue
  1. Assume transfer fee to be 5% and ticket prize is 1.5 Dai.
  2. Alice buys 100 of tickets using buyTickets function after approving 100 * 1.5 Dai = 150 Dai to Lottery contract.
  3. 150 Dai will be deducted from Alice account but Lottery contract will recieve 150 Dai * (100 - 5) / 100 = 142.5 = 142 Dai
  4. Lottery contract will clearly have a 5% loss on 100 tickets. It recieved 142 Dai instead of 150 Dai.

Tools Used

Manual Review

Recommended Mitigation Steps

Make sure to check how much Lottery contract is recieving while someone buy a ticket like shown below.

function buyTickets(
    uint128[] calldata drawIds,
    uint120[] calldata tickets,
    address frontend,
    address referrer
)
    external
    override
    requireJackpotInitialized
    returns (uint256[] memory ticketIds)
{
    if (drawIds.length != tickets.length) {
        revert DrawsAndTicketsLenMismatch(drawIds.length, tickets.length);
    }
    ticketIds = new uint256[](tickets.length);
    for (uint256 i = 0; i < drawIds.length; ++i) {
        ticketIds[i] = registerTicket(drawIds[i], tickets[i], frontend, referrer);
    }
    referralRegisterTickets(currentDraw, referrer, msg.sender, tickets.length);
    frontendDueTicketSales[frontend] += tickets.length;
    //@audit-fix-start
    uint256 balanceBefore = rewardToken.balanceOf(address(this));
    rewardToken.safeTransferFrom(msg.sender, address(this), ticketPrice * tickets.length); 
    uint256 balanceAfter = rewardToken.balanceOf(address(this));
    require(balanceAfter - balanceBefore >= ticketPrice * tickets.length);
    //@audit-fix-end
}
c4-judge commented 1 year ago

thereksfour marked the issue as unsatisfactory: Overinflated severity