neko_nyaa - Admin can unrestrictedly affect the odds of a raffle by setting themselves up with role(1) in `WinnablesTicket`

[sherlock-admin4]

neko_nyaa

High

Admin can unrestrictedly affect the odds of a raffle by setting themselves up with role(1) in WinnablesTicket

Summary

A core invariant defined in the contest README is that:

Admins cannot affect the odds of a raffle

While the centralization risk of admin self-minting tickets is noted, the following assumption is noted:

The existence of max ticket supply and max holdings however guarantees a minimum level of fairness

By setting themselves up with role(1) directly in the WinnablesTicket token contract, the admin can bypass all these assumptions (max ticket supply, max holdings), and affect the winning odds with no limit.

Root Cause

First of all, let's look at mint() in WinnablesTickets contract:

function mint(address to, uint256 id, uint256 amount) external onlyRole(1) {

https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/main/public-contracts/contracts/WinnablesTicket.sol#L182-L199

It is clear that role(1) can mint unlimited tickets. Furthermore, the admin can also grant themselves the role, bypassing any restrictions in buyTicket(). We now investigate the impact (how the results are affected by the admin minting tickets to themselves)

When the raffle results are created, the winner is calculated using the supply from the ticket storage.

function _getWinnerByRequestId(uint256 requestId) internal view returns(address) {
    RequestStatus storage request = _chainlinkRequests[requestId];
    uint256 supply = IWinnablesTicket(TICKETS_CONTRACT).supplyOf(request.raffleId); // @audit supplyOf is taken from the ticket
    uint256 winningTicketNumber = request.randomWord % supply;
    return IWinnablesTicket(TICKETS_CONTRACT).ownerOf(request.raffleId, winningTicketNumber);
}

https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/main/public-contracts/contracts/WinnablesTicketManager.sol#L472

By minting (virtually) unlimited tickets to themselves, the admin bypasses all restrictions imposed in ticket purchase, granting themselves victory odds far exceeding the restrictions imposed.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. The admin locks a prize, and starts a raffle as normal. People starts buying tickets to enter the raffle.
2. Admin grants themselves role(1) on the WinnablesTickets (not the ticket manager), and mints themselves (or any related party) almost unlimited tickets.
3. Admin bypasses all max ticket restrictions, and said party is virtually guaranteed to be the winner.

Impact

Admin can unrestrictedly affect the odds of a raffle, breaking protocol invariant

PoC

No response

Mitigation

No response

[neko-nyaa]

Escalate. As per the README, the admin should not have a method to tamper with the odds of drawing a raffle. This submission shows one such method.

[sherlock-admin3]

Escalate. As per the README, the admin should not have a method to tamper with the odds of drawing a raffle. This submission shows one such method.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Brivan-26]

Please note that #79 and #86 so far are dups of this

[rickkk137]

• First:

  If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules. In case of contradictions between the README and CODE COMMENTS, the README is the chosen source of truth

Sponsor clearly mention every admin's action which prevent winners to claim their prize is broken assumption

The principles that must always remain true are: Winnables admins cannot do anything to prevent a winner from withdrawing their prize

Second:

86

root cause:admin can send arbitrary message to WinnablesPrizeManager::_ccipReceive and can change winner

129 #79:

root cause:admin can change supply tokens after randomWord will be provided by chainlink node,

hence we cannot classified them with eachother because root causes and fixes are different

[0x-Shahil-Hussain]

482 is also a dup of this

[tejas-warambhe]

163 is dup similar to this.

[S3v3ru5]

The following two issues have the same impact as this one but different root cause.

289 - admin can send arbitrary message to WinnablesPrizeManager::_ccipReceive and can change winner by using _setCCIPCounterpart

271 - Admin can use integer overflow after the winner is declared to overwrite the winning ticket owner.

Mentioning them here as I cannot escalate them

[Joshuajee]

I doubt the validity of this though, there is an underlying assumption that the admin would never do this. Admin will never abuse the signature to mint free tickets for themselves or addresses that they control

[Brivan-26]

@Joshuajee You misunderstand, this issue is not about abusing the signature. the issue is about the admin minting directly the ticket by interacting with WinnableTickets directly after the random number is fulfilled. He can easily determine the winner after the randomword is known

[Joshuajee]

That was about a separate issue which I validated, this one is talking about buying tickets during the raffle which the admin won't do as mentioned in the readme. The one I validated has to do with the admin minting it after the winner has been selected i.e they are choosing winners not the VRF.

[johnson37]

#88 is dup similar to this.

[S3v3ru5]

That was about a separate issue which I validated, this one is talking about buying tickets during the raffle which the admin won't do as mentioned in the readme. The one I validated has to do with the admin minting it after the winner has been selected i.e they are choosing winners not the VRF.

@Joshuajee are you referring to this one #271 ?

[Joshuajee]

Yes exactly @S3v3ru5

[mystery0x]

All admin related issues have been discussed with the HOJ and the sponsor who wished the following governing statements could have been more carefully written and/or omitted:

"The protocol working as expected relies on having an admin creating raffles. It should be expected that the admin will do their job. However it is not expected that the admin can steal funds that should have ended in a raffle participant’s wallet in any conceivable way."

Much as many of the findings suggest good fixes, the conclusion is that they will all be deemed low unless it's beyond the trusted admin control.

[Joshuajee]

This is not what the Readme said and this is making the work of security researchers and people who judged the contest as wasted efforts.

[Brivan-26]

The issues submitted in this report clearly breaks the following invariant:

Admins cannot affect the odds of a raffle

after the random word is fulfilled—meaning after the fulfillRandomWords function is called and the random word is known. At this point, the admin can determine the random word and exploit it by calling the WinnablesTicket::mint function to mint additional tickets. By increasing the ticket supply, the admin can manipulate the outcome of the raffle, thereby controlling who wins

Check #79 for better details

[WangSecurity]

I agree this issue breaks the invariant from the README:

Admins cannot affect the odds of a raffle

And I don't think it breaks the following statement, i.e. there's no abuse of the signatures involved. The tickets are bought, but not due to the abused signature:

Admins will not abuse signatures to get free tickets or to distribute them to addresses they control

Hence, this issue is medium severity, planning to accept the escalation. This issue will be the main one. The issue will be based on the conceptual mistake of the admin being able to affect the odds of a raffle, therefore, the duplicates are also the issues about this invariant broken:

• 79

• 482

• 271

• 88

• 191

• 303

• 224

• 580

• 229

*Note: there are other similar reports (even mentioned above), most likely because they break the other invariant (about the admin preventing the winner from claiming their prize), or it may have been missed. In the second case, if you're confident your report is about affecting the odds of a raffle, please let me know.

[Brivan-26]

@WangSecurity #37 is NOT a valid duplicate (it talks about Using ERC721.transferFrom() instead of safeTransferFrom())

[rickkk137]

@WangSecurity I hope,this can be helpful

root cause:admin can change total supply and mint more ticket and finally change winner

• 79

• 482

• 271

• 88

• 303

• 224

• 337

• 580

root cause:admins can send arbitrary message to WinnablesPrizeManager contract and change winner in favor of themself

• 86

• 98

• 289

273 is not a valid duplicate

let's assume maxTicketSupply = 100 and maxHoldings = 100 and admin give signture to specific user to buy 100 ticket and that user buy all 100 ticket,hence other users cannot buy ticket

        unchecked {
            if (supply + ticketCount > raffle.maxTicketSupply) revert TooManyTickets();
        }

https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/81b28633d0f450e33a8b32976e17122418f5d47e/public-contracts/contracts/WinnablesTicketManager.sol#L414 becuase of this line its mean if a user can buy all tickets,other users cannot participate is that raffle

[0xshivay]

@WangSecurity Issue #580 is a valid duplicate of issue #129.

[Brivan-26]

@WangSecurity @rickkk137 #86 #98 #289 are dups of #277 not #129

All of the three mentioned issues above share the same root cause (updating the ccip counterpart to a contract controlled by the admin to control the winner) and they highlight different impacts of #277

The issue here is totally different: after the random word is fulfilled and the random word is known, the admin can call the WinnablesTicket::mint function to mint additional tickets. By increasing the ticket supply, the admin can manipulate the outcome of the raffle, thereby controlling who wins

273 is an invalid issue and describes an intended behavior (check my comment there)

[WangSecurity]

@rickkk137 @Brivan-26 Yes, you're correct, these issues should be duplicated with #277, excuse me for that. Also, correct about #273. Updated the duplication lists.

The decision remains, accept the escalation and apply the changes from my previous comments (it was edited and it's up-to-date).

[Brivan-26]

Thanks for your reconsideration @WangSecurity Can you provide more explanation as to why Medium severity is appropriate and not High? As far as I understood from your comment, your decision is based:

1. there's no abuse of the signatures involved.
2. The issue will be based on the conceptual mistake of the admin being able to affect the odds of a raffle

For the first point, indeed, there is no signature abuse, but it is not involved in the attack context. For the 2nd point, this issue is not about a mistake, but about intentional manipulation of raffle outcomes

The impact of this issue is affecting the odds of the raffles which prevents the real winner from receiving his rewards and thus losing the amount he sold tickets with (especially if he bought a large amount of tickets to increase his win chances). So, as per sherlock rules, this issue should be HIGH

[WangSecurity]

Can you provide more explanation as to why Medium severity is appropriate and not High?

This issue is valid only because it's breaking the invariant from the README that the admin cannot affect the odds of the raffle. If there were no such invariant, this would be invalid. Hence, it should be medium severity.

The decision remains as in this comment.

[0xsimao]

229 is a dup

[WangSecurity]

Result: Medium Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• neko-nyaa: accepted

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/Winnables/public-contracts/pull/20