sherlock-admin3 commented 1 month ago

0x73696d616f

Medium

Users buying too many tickets will DoS them and the protocol if they are the winner due to OOG

Summary

WinnablesTicket stores nft ownership by setting the first minted nft id ownership to the user minting and all the next minted nfts remain as 0. This means it always costs the same to mint, but the ownerOf() function becomes much more expensive, to the point where it may cause OOG errors. In this case, the user is able to buy tickets via WinnablesTicketManager::buyTickets(), the draw is made in WinnablesTicketManager::drawWinner() and the chainlink request is fulfilled with the winner in WinnablesTicketManager::fulfillRandomWords(). However, in WinnablesTicketManager::propagateWinner(), it reverts due to OOG when calling WinnablesTicket::ownerOf().

Root Cause

In WinnablesTicket:97-99, it may run out of gas if enough tickets were bought.

Internal pre-conditions

Admin sets max holdings and max tickets to a number of at least 4000 or simillar.

External pre-conditions

None.

Attack Path

Users by tickets via WinnablesTicketManager::buyTickets()`
The draw is made in WinnablesTicketManager::drawWinner()
The chainlink request is fulfilled with the winner in `WinnablesTicketManager::fulfillRandomWords()]
In WinnablesTicketManager::propagateWinner(), it reverts due to OOG when calling WinnablesTicket::ownerOf().

Impact

DoSed winner and protocol ETH from the raffle.

PoC

The following test costs 9 million gas, more than the block limit on Avalanche.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console} from "forge-std/Test.sol";
import {WinnablesPrizeManager, LinkTokenInterface} from "contracts/WinnablesPrizeManager.sol";
import {WinnablesTicket} from "contracts/WinnablesTicket.sol";
import {IRouterClient} from "@chainlink/contracts-ccip/src/v0.8/ccip/interfaces/IRouterClient.sol";
import {Client} from "@chainlink/contracts-ccip/src/v0.8/ccip/libraries/Client.sol";

contract WinnablesPrizeManagerTest is Test {
    WinnablesPrizeManager public winnablesPrizeManager;
    WinnablesTicket public winnablesTicket;
    LinkTokenInterface public link;
    IRouterClient public router;
    address public owner;

    function setUp() public {
        vm.createSelectFork(vm.envString("ETHEREUM_RPC_URL"));
        link = LinkTokenInterface(0x514910771AF9Ca656af840dff83E8264EcF986CA);
        router = IRouterClient(0x80226fc0Ee2b096224EeAc085Bb9a8cba1146f7D);
        owner = makeAddr("owner");

        vm.prank(owner);
        winnablesPrizeManager = new WinnablesPrizeManager(address(link), address(router));

        vm.prank(owner);
        winnablesTicket = new WinnablesTicket();
    }

    function test_POC_OwnerOfDos() public {
        vm.prank(owner);
        winnablesTicket.setRole(owner, 1, true);

        vm.prank(owner);
        winnablesTicket.mint(owner, 1, 4000);

        winnablesTicket.ownerOf(1, 3999);
    }
}

Mitigation

Set a reasonable cap for max holdings.

0xsimao commented 4 weeks ago

Escalate

This finding should be valid as it was not known and is outside what is considered admin responsability to set reasonable values (it is not simply a matter of array length, for example).

sherlock-admin3 commented 4 weeks ago

Escalate

This finding should be valid as it was not known and is outside what is considered admin responsability to set reasonable values (it is not simply a matter of array length, for example).

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.

kuprumxyz commented 4 weeks ago

413 is a duplicate of this, or vice versa; whatever fits. A couple more arguments of why this is a valid High:

In my finding I arrived at the estimate of 12608 tickets in a batch for this issue to be triggered; the present finding provides an even more precise value of ~4000, which is completely realistic even in the current system. Citing from my finding:

Long Running Raffle has max tickets 60000, out of which at the time of writing 7043 are sold, and the largest sold batch contains 4347 tickets.
Sherlock rules say:
How to identify a high issue:
- Definite loss of funds without (extensive) limitations of external conditions.

According to the rules, this is a valid High because:

The loss of funds is definite: the funds are locked permanently, as neither the winner can be drawn, nor the raffle can be canceled.
There are no limitations of external conditions: the bug will trigger by itself as long as the internal conditions are met (the winning ticket is at the end of a large enough batch of bought tickets).

DemoreXTess commented 3 weeks ago

331 is a duplicate of this

S3v3ru5 commented 3 weeks ago

349

mystery0x commented 3 weeks ago

This is being linked to https://github.com/sherlock-audit/2024-08-winnables-raffles-judging/issues/142 where the sponsor has disputed this finding alleging it would require admin mistake via the API misconfiguration allowing players to buy excessive amount of tickets, as already clarified in the discord channel.

As such, this finding will be low at best.

kuprumxyz commented 3 weeks ago

@mystery0x, I respectfully disagree.

According to Sherlock's rules, this is a valid High: see my previous comment on that.
What sponsor says about an external API is irrelevant for the contest:
- What's audited are the contracts, not the admin's API. API has not been made accessible or even known to Watsons. The contracts do allow this behavior.
- Before this finding, the restrictions on the number of tickets was not imposed by the API, or even known to the admins, as follows from sponsor's comments on Discord.
- In any case, what the sponsor says now, after the fact, about these restrictions, is irrelevant. What is relevant is that according to Sherlock's hierarchy of truth the number of bought tickets in one transaction could be type(uint16).max.
Here is the relevant information from the README:

Q: Are there any limitations on values set by admins (or other roles) in the codebase, including restrictions on array lengths? None other than the restrictions inherent to the types used (for example: WinnablesTicketManager::buyTickets() take a uint16 argument for ticketCount. That means the maximum number of tickets that can be purchased in a single transaction is type(uint16).max)

Brivan-26 commented 3 weeks ago

Buying tickets is an off-chain mechanism, if a user can buy too many tickets to DoS the ownerOf function, it would be because of the API allowing so, which is Out of scope

DemoreXTess commented 3 weeks ago

@Brivan-26 @kuprumxyz

This message clarify everything. I believe this README proves it to validate this issue.

@mystery0x, I respectfully disagree.

According to Sherlock's rules, this is a valid High: see my previous comment on that.

What sponsor says about an external API is irrelevant for the contest:

What's audited are the contracts, not the admin's API. API has not been made accessible or even known to Watsons. The contracts do allow this behavior.

Before this finding, the restrictions on the number of tickets was not imposed by the API, or even known to the admins, as follows from sponsor's comments on Discord.

In any case, what the sponsor says now, after the fact, about these restrictions, is irrelevant. What is relevant is that according to Sherlock's hierarchy of truth the number of bought tickets in one transaction could be type(uint16).max.

Here is the relevant information from the README:

Q: Are there any limitations on values set by admins (or other roles) in the codebase, including restrictions on array lengths? None other than the restrictions inherent to the types used (for example: WinnablesTicketManager::buyTickets() take a uint16 argument for ticketCount. That means the maximum number of tickets that can be purchased in a single transaction is type(uint16).max)

Brivan-26 commented 3 weeks ago

The only statement that could validate this issue is:

Before this finding, the restrictions on the number of tickets was not imposed by the API, or even known to the admins, as follows from sponsor's comments on Discord.

However, as the sponsor said, we are not auditing the API. The ticket purchase flow is very clear, users buy tickets from the API, the API generates the signatures and then the users call the smart contract.

So, the purchase is happening off-chain and the root reason for this finding is the fact that users can buy too many tickets. The root reason is off-chain

kuprumxyz commented 3 weeks ago

@Brivan-26 you are completely overturning the reasoning. The Watsons, when they are participating in a contest, are not aware of anything besides what's stated in Sherlock's hierarchy of truth. If there were any restrictions to the inputs, they should have been made known in the README. If this is not done (and in fact as we see the opposite is done, specifying type(uint16).max) as the range) ==> this is the valid finding.

If we followed your reasoning, absolutely any finding can be invalidated by a sponsor after the contest: they would come and say "There is an API that will restrict the inputs to this and that function, and the finding is no more". Let me repeat here the allegory I have already written elsewhere:

A careless person drives a car, holding the steering wheel with one hand, and with the other hand chatting on a smartphone. The policeman notices that driver's eyes are not on the road, but on a phone, and that there is a tree trunk right ahead; so the policeman whistles sharply to prevent the car crash. The driver breaks sharply, leaves the car, and instead of thanking the officer says "You sharp whistle scared me to death! Why were you whistling?!? I was holding the steering wheel, my foot was on the breaking pedal, I can always break in time!"

The officer was seeing what he was seeing: a car driving right into the tree trunk, and the driver looking not ahead of him, but on the phone. The officer had no evidence that the person was even aware of the tree ahead; all evidence was to the opposite; so he was right to whistle and prevent an accident.

Brivan-26 commented 3 weeks ago

Hey @kuprumxyz I didn't say at all that the sponsor can invalidate the finding. I'm just referring to how ticket purchase works. If users can buy tickets in the smart contracts, I'd totally agree with you that this is a valid issue. But, the root cause here does not happen in the smart contracts (buying too many tickets) but in an off-chain API.

I said my arguments here, I can't provide more.

kuprumxyz commented 3 weeks ago

Hey @Brivan-26, the root cause happens exactly in the smart contracts:

The root cause is in WinnablesTicket::ownerOf();
which is triggered by buying tickets via WinnablesTicketManager::buyTickets();
and the valid input to this function is type(uint16).max, as specified directly in the README:

WinnablesTicketManager::buyTickets() take a uint16 argument for ticketCount. That means the maximum number of tickets that can be purchased in a single transaction is type(uint16).max

That's it. I also have nothing to add.

Brivan-26 commented 3 weeks ago

The root cause is not in ownerOf, the impact is located on ownerOf (OOG)
Buying tickets is not via buyTickets function, buying tickets happens off-chain, users just provide the signature generated by the API on buyTickets to claim the tickets. I don't know why you keep avoiding this fact

kuprumxyz commented 3 weeks ago

@Brivan-26 I am tired of this discussion tbh... we are running in circles. If you want, take a look at my finding #413 for the explanation why ownerOf is the root cause (and how to refactor it to mitigate the issue), and why buyTickets is the internal precondition.

I would kindly ask @WangSecurity to offer their view; and will refrain from any further comments until then.

WangSecurity commented 2 weeks ago

I'm not sure I understand that part about the API. The README says:

Ticket purchases need to be approved by the API which grants a signature

It doesn't say it will not allow users to buy big batches of tickets. Moreover, we have the following line:

That means the maximum number of tickets that can be purchased in a single transaction is type(uint16).max)

Hence, each user can buy as many tickets as they want, even though they have to be approved by the API. @Brivan-26 I may need your clarification as to why you think this issue is invalid.

Brivan-26 commented 2 weeks ago

@WangSecurity The root reason for this issue is users buying as many tickets as they want, correct? Now:

the root reason is the purchase of the tickets, happens on the API => off-chain
The mitigation is restricting how many tickets a user can purchase, where does this mitigation take place? Again, Off-chain

I can't provide more than the above two points

debugging3 commented 2 weeks ago

Internal pre-conditions of this report is:

Admin sets max holdings and max tickets to a number of at least 4000 or simillar.

Sherlock Rule is:

Exception: In case the array length is controlled by the trusted admin/owner or the issue describes an impractical usage of parameters to reach OOG state then these submissions would be considered as low.

From the above, Setting max holdings and max tickets smaller than 4000 is the admin's responsibility. Therefore this report should be considered as low.

Oblivionis214 commented 2 weeks ago

Exception: In case the array length is controlled by the trusted admin/owner or the issue describes an impractical usage of parameters to reach OOG state then these submissions would be considered as low.

This statement is overriden by

III. Sherlock's standards: Hierarchy of truth: If the protocol team provides no specific information, the default rules apply (judging guidelines).

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.

Brivan-26 commented 2 weeks ago

@Oblivionis214 The README did not override the following rule:

In case the array length is controlled by the trusted admin/owner or the issue describes an impractical usage of parameters to reach OOG state then these submissions would be considered as low

The API is still controlling the number of tickets a user can purchase. Check this comment

DemoreXTess commented 2 weeks ago

@WangSecurity

While @Brivan-26 makes a valid point regarding this submission, I still believe the issue is valid due to the contents of the README file. As @kuprumxyz noted, the README explicitly mentions a possible limit on the number of tickets in the buyTicket function.

kuprumxyz commented 2 weeks ago

I didn't want to interfere anymore, but being called upon by @DemoreXTess I need to... Having

III. Sherlock's standards: Hierarchy of truth: If the protocol team provides no specific information, the default rules apply (judging guidelines). 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.

and then

@Oblivionis214 The README did not override the following rule:

In case the array length is controlled by the trusted admin/owner or the issue describes an impractical usage of parameters to reach OOG state then these submissions would be considered as low

The API is still controlling the number of tickets a user can purchase. Check this comment

@Brivan-26 you arrived at the logical contradiction which is:

how is it possible that "If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules."
and then you say "The README did not override the following rule ..."?
All means every single one.

Brivan-26 commented 2 weeks ago

@kuprumxyz There is no contradiction. I said that the README or CODE COMMENT did not override the following rule:

In case the array length is controlled by the trusted admin/owner...

The number of tickets a user can purchase is still controlled by the API that is controlled by the admin.

Oblivionis214 commented 2 weeks ago

Hierarchy of truth: If the protocol team provides no specific information, the default rules apply (judging guidelines).

If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules.

Readme did override the rule about array.

WinnablesTicketManager::buyTickets() take a uint16 argument for ticketCount. That means the maximum number of tickets that can be purchased in a single transaction is type(uint16).max

This means protocol team want to inform watsons the maximum "array length" in protocol design. So anything below this value should be considered possible.

WangSecurity commented 2 weeks ago

To confirm, if I understand correctly, you talk about the _ticketOwnership mapping when referring to the array. And it can be controlled by an admin to set max holdings and max tickets to a value that wouldn't cause an OOG issue. That's a fair assumption. But, we have the following line:

WinnablesTicketManager::buyTickets() take a uint16 argument for ticketCount. That means the maximum number of tickets that can be purchased in a single transaction is type(uint16).max

Thus, it's a possible scenario that the user can buy up to 65535 tickets, and we should consider this: even though it's done off-chain through API, the user is still able to buy this many tickets. Hence, I believe this should be a valid issue with medium severity due to the conditions that the user should buy lots of tickets (more than 4000, since the report is imprecise and the block gas limit on Avalanche is 15 limit IIUC) and the winning ticket should be near to the last ticket. Hence, I believe medium is more appropriate.

Planning to accept the escalation. The duplicates are:

413
331
349
339
142

Special thanks to @Brivan-26 for telling on other issues that the main discussion is here.

@mystery0x are there missing duplicates?

neko-nyaa commented 2 weeks ago

431 is not a dupe. That issue was always judged correctly.

kuprumxyz commented 2 weeks ago

@WangSecurity,

I agree, #431 is not a dupe. I should also note that per Sherlock's rules, #339 is not a dupe either, as it doesn't include a coded PoC.

We have Requirements:

PoC is required for all issues falling into any of the following groups:

non-obvious ones with complex vulnerabilities/attack paths

issues for which there are non-trivial limitations/constraints on inputs, to show that the attack is possible despite those

issues related to precision loss

reentrancy attacks

attacks related to the gas consumption and/or reverting message calls

Also we have Duplication rules:

The duplication rules assume we have a "target issue", and the "potential duplicate" of that issue needs to meet the following requirements to be considered a duplicate.

Identify the root cause

Identify at least a Medium impact

Identify a valid attack path or vulnerability path

Fulfills other submission quality requirements (e.g. provides a PoC for categories that require one)

Only when the "potential duplicate" meets all four requirements will the "potential duplicate" be duplicated with the "target issue", and all duplicates will be awarded the highest severity identified among the duplicates.

Otherwise, if the "potential duplicate" doesn't meet all requirements, the "potential duplicate" will not be duplicated but could still be judged any other way (solo, a duplicate of another issue, invalid, or any other severity)

neko-nyaa commented 2 weeks ago

Disagree. The rule never states that the PoC has to be coded. Submission #339 has shown all the calculations needed to prove that the number in range can cause a DoS, from Avalanche's block gas limit, to the cost of each storage read, down to the number of tickets that can be purchased, showing that a value in the range can indeed cause the issue, which is sufficient as a proof.

Brivan-26 commented 2 weeks ago

@WangSecurity

To confirm, if I understand correctly, you talk about the _ticketOwnership mapping when referring to the array. And it can be controlled by an admin to set max holdings and max tickets to a value that wouldn't cause an OOG issue. That's a fair assumption. But, we have the following line: WinnablesTicketManager::buyTickets() take a uint16 argument for ticketCount. That means the maximum number of tickets that can be purchased in a single transaction is type(uint16).max

Can you explain how this scenario is an exception of the following Sherlock rule:

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

The number of tickets that can be purchased is still controlled by the API which is controlled by the admin. So, if the admin sets that large number that causes the OOG, how this is different from other contests in which such actions are judged invalid? It doesn't matter if the sponsor acknowledged that the tickets that can be purchased is very large, the input is still controlled by the admin

kuprumxyz commented 2 weeks ago

Disagree. The rule never states that the PoC has to be coded. Submission #339 has shown all the calculations needed to prove that the number in range can cause a DoS, from Avalanche's block gas limit, to the cost of each storage read, down to the number of tickets that can be purchased, showing that a value in the range can indeed cause the issue, which is sufficient as a proof.

yeah, may be you are right. I agree that your calculations are enough of a proof.

It's not clarified in the rules how the PoC should look like, and I am used to think about a PoC as a coded one. @WangSecurity, can we get a clarification what is actually understood by PoC in the rules?

Oblivionis214 commented 2 weeks ago

If a coded poc to show gas comsumption is needed, #331 also has no POC

kuprumxyz commented 2 weeks ago

If a coded poc to show gas comsumption is needed, #331 also has no POC

I was reading #331 for the second time, and realized that I was mistaken previously: it does talk about the same issue as this one.

It's true though that it doesn't have a coded PoC, and also doesn't have proper calculations as in #339 to explain why an OOG would happen; it has only a code path/walkthrough that somehow explains the problem. So it all really depends on the definition of a PoC.

Let's wait for judge's input.

DemoreXTess commented 2 weeks ago

The coded PoC is not required for obvious issues. uint16.max limit is really high for a for loop especially while working with storage variables. I believe both #331 and #339 should be accepted because both of them states the problem accurately.

kuprumxyz commented 2 weeks ago

On the one hand, I tend to agree. On the other, specifically because of the rules, I've spent time to write a coded PoC (otherwise I would not). I would like at least for the future to obtain a clarification whether I need to do it, or not.

WangSecurity commented 2 weeks ago

The number of tickets that can be purchased is still controlled by the API which is controlled by the admin. So, if the admin sets that large number that causes the OOG, how this is different from other contests in which such actions are judged invalid? It doesn't matter if the sponsor acknowledged that the tickets that can be purchased is very large, the input is still controlled by the admin

Thank you, it's a great question. I'll try to explain, but let me know if you still have questions. Even though the input is still controlled by the admin, still there can be type(uint16).max tickets bought in one transaction. But here, the issue doesn't arise due to admin setting the maxTickets and maxHoldings to a high, the code can work with them perfectly, unless one user buys too many tokens in such a scenario. Hence, it's not the admin action that breaks the code, that's why i don't apply the rule.

Regarding the POC, indeed the POC can be a very detailed texted one and the coded POC would be ideal, but not mandatory. All the issues have either texted or a coded one, while not of the are perfect, still they're sufficient.

The decision remains, accept the escalation and validate with medium severity.

WangSecurity commented 1 week ago

Result: Medium Has duplicates

sherlock-admin4 commented 1 week ago

Escalations have been resolved successfully!

Escalation status:

0xsimao: accepted

sherlock-admin2 commented 1 week ago

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

sherlock-audit / 2024-08-winnables-raffles-judging

0x73696d616f - Users buying too many tickets will DoS them and the protocol if they are the winner due to OOG #398

Users buying too many tickets will DoS them and the protocol if they are the winner due to OOG

Summary

Root Cause

Internal pre-conditions

External pre-conditions

Attack Path

Impact

PoC

Mitigation

413 is a duplicate of this, or vice versa; whatever fits. A couple more arguments of why this is a valid High:

331 is a duplicate of this

349

413

331

349

339

142

431 is not a dupe. That issue was always judged correctly.