Open hats-bug-reporter[bot] opened 1 month ago
I see you made a higher effort issue
Pasting the link to the response I gave to your previous issue: https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/issues/99#issuecomment-2378149352
@greenlucid @clesaege Want to answer comments on issue #99 here, as this one is better structure report:
@greenlucid
It doesn't revert during reportPayouts (I didn't test if it does, I'm taking your word for it @0xmahdirostami) A reward for the high option after a low outcome with such large ranges, would be around 1/1e70. That's zero, and it will only trigger when redeeming tokens. The redemption function allows the user to choose the shares they're redeeming, and since they'd get zero (or dust at best) they can prevent themselves from redeeming it, allowing the tx to not revert.
The revert will happen while redemption:
If a market is created with an upperBound of 2^256 - 3
, lowerBound of 1
and the resolved answer is 10
, the resulting payouts will be:
upperBound - answer = 2^256 - 3 - 10
answer - lowerBound = 10 - 1
payouts[0] = 2^256 - 13
payouts[1] = 9
UP tokens redeem for (value - min)/(max - min)
DOWN tokens redeem for (max - value)/(max - min).
So the issue will be for Down tokens: user balance * 2^256 - 13 / 2^256 - 4
will revert.
@clesaege :
This is pretty different than for multiscalar. For multiscalar, there isn't a specified upper bound, so if the result is very high, that would have caused an issue. So we should handle reality potentially giving very high values, but we don't need to prevent at the contract level someone from giving in very high upper bounds. Here you would need to make a market with a crazy high upperbound: 115792089237316195423570985008687907853269984665640564039457.584007913129639933 which would be a clear giveaway. This would be akin to reporting a vuln in any ERC20 which doesn't introduce a cap on the max supply.
As per contest rules, are excluded:
Issues about being able to create malicious markets (for example, you can create markets without using the MarketFactory with a malicious arbitrator, create child markets to it) as long as this wouldn't result in those being displayed in the interface looking like a normal markets (if a child market of a malicious market is displayed, but points to some market which is not displayed, cannot be interacted with or is labelled as problematic, we consider it fine as it would not get verified on Curate). Issues where the user is harming itself by interacting improperly with the contracts. It's possible for the users to call functions with improper parameters, incompatible tokens, etc but we assume that functions are called properly similarly to how they are called in the frontend. It's the job of smart contracts to protect against malicious behaviours, but it's the job of the frontend to protect against stupid ones! Any issue that is only theoretical but can't happen in practice.
There is no information about the range of the upper bound in the document or contract. This issue must be prevented at the contract level. Users could create a valid market with a high upper bound, and it's the contract's responsibility to handle these bounds. If the contract allows an upper bound less than max256-2, that means there is no problem with that.
Malicious market, harming, theoretical: Imagine a market where the question being discussed is a statistic or math-related question that requires a high upper bound. There are no malicious acts involved, and it's a completely valid market. There is no harm involved in this scenario, and this situation could potentially occur.
If you are unwilling to verify the upper bound in the factory contract, you have two other options:
upperBound - lowerBound
is less than type128.max
.payouts[0] = upperBound - answer = 2^256 - 3 - 10
payouts[1] = answer - lowerBound = 10 - 1
The issue is obvious as it is. It's possible to create a valid market related to math where the upper and lower bounds are in bytes32. In bytes32, which has 32 characters, the upper bound can be a big number. The market creator can convert bytes32 to unit256 and create this valid market, and users can buy these outcomes. The protocol is written with high quality and cares about the users' redemption process. If there is a chance that the payout will overflow (which shouldn't happen), the contract must prevent it.
There is no information about the range of the upper bound in the document or contract. This issue must be prevented at the contract level. Users could create a valid market with a high upper bound
Because there isn't a specific number, the range at which it could overflow depends of the quantity of tokens in existence. If we have range*totalSupply we can have an overflow. Since totalSupply of sDAI is not fixed, there is no number (other than 1) which can guarantee that there couldn't be in theory an overflow.
We are well aware that people were gonna make those kind of reports and make sure to insist on those kind of thing in the exclusions.
A market with a range of 59 digits is definitely not gonna look like a normal market. Anyone would be crazy to interact with that and will not pass the curation.
There is no legitimate use in making markets with ranges that wide. Scalar markets are for stuff like "how many votes will this candidate get?", "what would be the profit of this company for this year?".
It's a nice theorical report, but there is no practical issue there.
@clesaege
I'm not talking about an attack here, it could happen normally.
It's possible to create a valid market related to math where the upper and lower bounds are in bytes32. In bytes32, which has 32 characters, the upper bound can be a big number. The market creator can convert bytes32 to unit256 and create this valid market, and users can buy these outcomes. The protocol is written with high quality and cares about the users' redemption process. If there is a chance that the payout will overflow (which shouldn't happen), the contract must prevent it.
A user with, for example, 100 winning outcomes, may encounter difficulties when redeeming them.
100 10^18 high range > uint256 max
So you don't want to prevent it.
why do you think that all questions will have low range? there are a lot of samples of questions that could have high range.
why do you think that all questions will have low range? there are a lot of samples of questions that could have high range.
Which kind would you have in mind?
A user with, for example, 100 winning outcomes, may encounter difficulties when redeeming them. 100 10^18 high range > uint256 max
I think you mean 100 winning outcome tokens. When we generalize it to x winning outcome tokens (and talk in term of basic units to not have to deal with the decimals) we obtain an overflow if
Since x maximum value is also uint256_max (maximum amount of ERC20 tokens that someone could have), if we wanted to prevent theorical overflow at contract level, we would have:
Which would make the scalar markets not usable.
@clesaege
you are using uint256.max instead of uint128.max to establish an a range between upperbound and lowerbound for market creators. However, your calculation does not support uint256.max (it's like giving a high option that contract couldn't fulfil)
The uint256.max value is utilized in various fields: From statistics, Crypto, Drugs Market(thinks related to DNA), …
Currently, you are using sDai and the user balance will be under uint128.max.
https://gnosisscan.io/token/0xaf204776c7245bf4147c2612bf6e5972ee483701#readContract#F24
so in the redemption calculation if you have
(uint128.max-1) * range(uint128.max) < uint256.max you will prevent overflow.
You just need to be sure that range is under uint128.max
* We set maxPayout to a sufficiently large number for most possible outcomes that also avoids overflows in the following places:
* https://github.com/gnosis/conditional-tokens-contracts/blob/master/contracts/ConditionalTokens.sol#L89
* https://github.com/gnosis/conditional-tokens-contracts/blob/master/contracts/ConditionalTokens.sol#L242
*/
uint256 maxPayout = 2 ** (256 / 2) - 1;
this is the same as here,
here you just control the range to not exit 128.max
the mitigation is simple:
Verify that upperBound - lowerBound is less than type128.max.
Currently, you are using sDai and the user balance will be under uint128.max.
What guarantees that? I don't think it will exceed uint128.max in the near future, but my point is that when we are just analyzing operations from a theoretical perspective, and want strong theoretical guarantees, we end up with something which is unusable in practice.
The uint256.max value is utilized in various fields: From statistics, Crypto, Drugs Market(thinks related to DNA), …
The fact that you don't have any example of this shows that this type of report is handled by the "Any issue that is only theoretical but can't happen in practice." exclusion.
The fact that you don't have any example of this shows that this type of report is handled by the "Any issue that is only theoretical but can't happen in practice." exclusion.
@clesaege With the help of OpenAI.
Here are some prediction-based questions where the upper bound - lower bound exceeds the uint128
max (which is 3.4 × 10^38) but the upper bound remains below the uint256
max (which is 1.16 × 10^77). These questions focus on fields such as computing, astronomy, and combinatorial predictions.
uint128
max but less than uint256
max.uint128
max and less than uint256
max.uint128
max but is well below uint256
max.uint128
max but less than uint256
max.uint128
max and remains below uint256
max.uint128
max while being below uint256
max.These prediction questions are designed to explore hypothetical future scenarios in fields such as quantum computing, chess, biology, finance, and astronomy, ensuring that the difference between the upper and lower bounds exceeds the uint128
max but remains below the uint256
max.
Here are some prediction-based questions where the difference between the upper and lower bounds exceeds uint256.max / 10^10
(approximately 1.1579209 × 10^67) but the upper bound remains below uint256.max
(which is approximately 1.1579209 × 10^77). These examples span various fields, including technology, biology, astronomy, and finance.
uint256.max / 10^{10}
and remains below uint256.max
.uint256.max / 10^{10}
while being less than uint256.max
.uint256.max / 10^{10}
and is less than uint256.max
.uint256.max / 10^{10}
and remains below uint256.max
.uint256.max / 10^{10}
and is less than uint256.max
.uint256.max / 10^{10}
and remains below uint256.max
.These prediction questions span various fields and are structured to ensure that the difference between the upper and lower bounds exceeds uint256.max / 10^{10}
while keeping the upper bound below uint256.max
.
these are just some examples that the issue could happen in the contract. the contract will handle these bounds until the redemption process.
Currently, you are using sDai and the user balance will be under uint128.max.
What guarantees that? I don't think it will exceed uint128.max in the near future, but my point is that when we are just analyzing operations from a theoretical perspective, and want strong theoretical guarantees, we end up with something which is unusable in practice.
about this part, so i have to say if you think this way, you will face overflow in multiscalar markets.
* We set maxPayout to a sufficiently large number for most possible outcomes that also avoids overflows in the following places:
* https://github.com/gnosis/conditional-tokens-contracts/blob/master/contracts/ConditionalTokens.sol#L89
* https://github.com/gnosis/conditional-tokens-contracts/blob/master/contracts/ConditionalTokens.sol#L242
*/
uint256 maxPayout = 2 ** (256 / 2) - 1;
but in your usage of tokens, this issue will not appear for sDAI, STETH, so you just need to wonder about range part. my issue isn't theoretical and there is a chance to happen.
All of those are very contrived and not practical markets (and you needed an AI to satisfy this mental exercise).
about this part, so i have to say if you think this way, you will face overflow in multiscalar markets.
I'm not denying that, what I'm saying is that if do this sort of purely theoretical reasoning, we can't do anything practical.
So in conclusion, yes, you can create an overflow in a theoretical market using aberrent values but:
@clesaege
In that issue, the concern lies with the upper bound, while my focus is on the range between the upper bound and the lower bound.
This is not gonna get verified nor have people trading on it
I did not find any mention of this behaviour in the documentation or repository that I audited.
I read Verified Markets and Market Policy, but nowhere does it say that a market with a high range between upperBound
and lowerBound
is invalid.
https://cdn.kleros.link/ipfs/QmfGcodBzG53DBxS2Uxu5jug9YiHUpJ5tKFqhP6Z2HjscU/Seer%20-%20Verified%20Markets.pdf
https://cdn.kleros.link/ipfs/QmW6npv4J3jvF87WVCEaQQ6f2PK1T9ytX2eYg86XkT9ScK/Seer%20-%20Markets%20Policy.pdf
BTW, in your code, there is no check for it, so all these show that you were not having problem with this range between upper-bound and lower-bound. https://seer-pm.netlify.app/#/create-market is checked that doesn't allow the upper-bound lower than lower-bound, but ok with any range.
So there is only one remaining wonder here, is it practical or just theoretical? is there any question that the answer range could be varied more than type128.max?
Company X wants to launch a new meme coin (Pepe coin) that has MaxTotalySupply
in uin256.max, and has a high decimal value, this coin will launch on date 2025,01,01, and The total supply of the token will be based on the interactions that they will get or any other element.
Ex:
What will be the total supply of Pepe coin with address 0x0...
at 2025.01.02?
Lower Bound: 1e35
Upper Bound: 1e50
if users believe that the answer will be so large will buy upper bound. if users believe that the answer will be so low will buy upper bound.
But the company expects that answer will be between two bounds.
Or any other value. For these bounds, the difference is more than uint128.max
As you can see, this could be a practical consideration rather than just a theoretical one.
btw, the front could use e
notation to not show some big numbers in the front end.
want to add another point here:
In multiscalar market, the contract assumes that the question could have an answer more than uint128.max, so it shows that the contract is on the handling of an edge-cases.
So these questions could be made in scalarMarket as well, the question could have a really large range between upperbound and lowerbound.
A log market would be better here. Instead of answering 1e50 (since 5e49 is the average), answer is logarithmic. e.g. 42.2. Notice how the way these type of ranges and statistics line up, incentives for the way scalar markets are setup break, unless log answers are used. Because it's too risky to answer something in the low ranges
im just giving examples, the root issue is that you must be sure that the second part in the calculation of payoutStake.mul(payoutNumerator)
is less than uint128.max. exactly in the way that you handled max payout in multiscalar market.
@clesaege
In that issue, the concern lies with the upper bound, while my focus is on the range between the upper bound and the lower bound.
This is not gonna get verified nor have people trading on it
I did not find any mention of this behaviour in the documentation or repository that I audited. I read Verified Markets and Market Policy, but nowhere does it say that a market with a high range between
upperBound
andlowerBound
is invalid. https://cdn.kleros.link/ipfs/QmfGcodBzG53DBxS2Uxu5jug9YiHUpJ5tKFqhP6Z2HjscU/Seer%20-%20Verified%20Markets.pdf https://cdn.kleros.link/ipfs/QmW6npv4J3jvF87WVCEaQQ6f2PK1T9ytX2eYg86XkT9ScK/Seer%20-%20Markets%20Policy.pdf
As stated in the exclusions, the curation rules are not finalized and are out of scope:
You can see on #8, that I already stated that markets with crazy values is not something we would allow:
Yeah, the code did reserve the last 3 slots, but even type(uint256).max - 2 is way too high to make any sense (we'd end up around 1E77 and dividing by that would lead to 0, and a market with a maximum value of 1E77 would be obviously wrong and not pass curation).
In the exclusion, I took the example of a market pointing to a malicious market as parent, which is also not something present in the rules displayed on the front as those are not up to date, nor finalized. The goal of this competition is to break the code of the smart contracts (which are ready for review unlike the policies).
Since I see lot of discussions here, let's do a summary of all the reasons this is invalid and covered by multiple exclusions:
As per competition rules, are excluded:
Here a market with such a high maximum value would not be supported by the frontend. The frontend supports values up to 100,000,000,000,000,000,000, after that it stops being able to track exact values and gives some approximate value using the e notation.
Ex: You can see that it doesn't give the exact value (see how the 5 is gone missing) but an approximate value.
So for this exclusion to apply, we need two things:
The market can't be redeemed, so it is definitely malicious. The market is displayed with an approximate e notation. This is different than the display of normal markets (normal markets ranges are displayed with numbers and displayed exactly).
You can see that I already indicated that before while discussing another issue #8.
a market with a maximum value of 1E77 would be obviously wrong and not pass curation.
As per competition rules, are excluded:
You put is as high, so I think you argue that it fulfils the high requirement "Issues that lead to significant losses of funds.". For that to happen, there would need to:
For the first one, this is fulfilled.
For the second, it isn't.
Similar point as the previous section.
You did give 6 example of markets with the following ranges:
They all (beside 5. which is excluded) have orders of magnitudes between the lower and upper range of 1E40 or more. They are markets about exponential phenomenons. Scalar markets with linear units are not suited for those kinds of markets. Indeed the UP tokens of those markets would be almost worthless as they would only redeem for something non negligeable (let's say more than 0.01) if the end value is within 1E2 of the upper range. For example in your 1. if we have models of 1E67 parameters, which is in the upper part of the the range, the UP tokens would only redeem for 0.001.
Despite using AI to try to display some theoretical use, none of those are practical. No one would trade on those markets unless they miss understand those as if they were trading on the the exponent and not the actual value (i.e. thinks that if the result of 1 is 1E67, they'd get (67-13)/(70-13)=0.95 for UP tokens). And any missunderstanding would be out of scope "Issues about being able to create misleading markets".
This is not an issue, but even if we were to assume it was, it would be excluded by the following rules
Reporters will not receive a bounty for: Any known issue
As answered during a previous report #8 (note that I answered immediately before your report submission):
we'd end up around 1E77 and dividing by that would lead to 0, and a market with a maximum value of 1E77 would be obviously wrong and not pass curation
Such a high value would be likely to cause issues in other part of the code
So it is known, from before the report, that markets with crazy high values would not work. My reasoning was a bit different, it was that if we were to divide by a very high value, we'd get 0 (or potentially 1 if the value we divide is very high too) and that would not allow to redeem (as redeeming 1 obviously would not even be worth the gas spent).
If the curation rules were in scope, that would definitely be finding about those being incomplete. However, the verified market rules are incomplete and basically date of a time where it was only looking at categorical markets (the point 13. is mentioning multiscalar markets but it's not even right as the way the code is constructing the questions of multiscalar markets ensure that they are adaptation of the main question, so this rule is redundant with the code).
As per competition rules, are excluded:
We do appreciate the effort in looking at the code and searching for those theoretical things. However, this is not the goal of the bounty. The goal of the bounty is to find practical issues on smart contracts. Here the issues is:
@clesaege thank you user for the full response.
Yes, you are right. I just checked the frontend, and it seems that it couldn't handle values correctly. Actually, it's not malicious behavior, just the frontend couldn't handle it.
The market can't be redeemed, so it is definitely malicious.
The market can't be redeemed because of wrong way of handling calculations, not the market itself. Overflow in the calculation is due to the wrong way of handling calculation, not the market creator's faulty action.
Yes, you are right you added that comment before my issue, but that comment is not completely right.
even type(uint256).max - 2 is way too high to make any sense (we'd end up around 1E77 and dividing by that would lead to 0,
Your calculation is max or low
- answer * balance / max - min, so if it return zero it will return full amount for winner part
again the issue is not about higher upper bound, the issue is overflow about high upperbound - lowebound
You put is as high, so I think you argue that it fulfils the high requirement "Issues that lead to significant losses of funds.". For that to happen, there would need to:
actually not, i put it as high because of:
Long-term freezing of user funds (ex: preventing a market to resolve for 100 years). Significant protocol insolvency (ex: letting users redeem more assets than they should such that the last users are unable to redeem).
Winner unable to redeem their funds
There is no practical legitimate use
AGAIN YES, you are right, but we could have better upper and lower bounds. ex: 1e76, 2e76
As an auditor, I want to emphasize that during my code audit, I identified a potential overflow risk in the code due to the lack of checking for 'upperBound - lowerBound'. It is the code's responsibility to prevent this scenario in the first place. Additionally, the front end also shares the responsibility to address this issue.
My issue was a mathematic vulnerability, and the issue will exist in code, even if you prevent it in the front end.
Yes, you are right you added that comment before my issue, but that comment is not completely right.
even type(uint256).max - 2 is way too high to make any sense (we'd end up around 1E77 and dividing by that would lead to 0,
Your calculation is max or low - answer * balance / max - min, so if it return zero it will return full amount for winner part the issue is not about higher upper bound, the issue is overflow about high upperbound - lowebound
I mean if you divide something by uint.max-2, it will return 0, because of the way solidity division truncates. Except for values >= uint.max-2, but due to the left value being a uint, there is only uint.max-2, uint.max-1, uint.max which are >= uint.max-2. So at best it could return 1, which is obviously negligible and it's not practical gaswise to redeem 1 by 1.
So yeah my initial comment was not exactly right in theory as it can return 1 (not just 0 as my initial comment was stating), but in practice, 1 basic uint of the token (here 0.000000000000000001 sDAI) is almost 0.
again the issue is not about higher upper bound, the issue is overflow about high upperbound - lowebound
To have a high upperbound - lowebound you necessarily need to have a high upperbound.
As an auditor, I want to emphasize that during my code audit, I identified a potential overflow risk in the code due to the lack of checking for 'upperBound - lowerBound'. It is the code's responsibility to prevent this scenario in the first place.
I think that's where the huge disagreement lies, are excluded:
Having the smart contracts only protect about malicious behaviours is a core element of Kleros smart contract development philosophy (also used by Seer.) And that's really a hill I'll die on, as this is what we've been following (+bounties, initially we were doing those on Solidified) since 2018 and allowed us to have a perfect record of never have had any exploit in all projects we've released.
It's not possible, nor advised, to protect against all types of stupid behaviours. A good example is token sending: Tokens don't prevent people to send them to an invalid address (and it's not due to a lack of trying, I've talked to 3 projects who were basically trying to fix the issues of sending tokens to wrong address at the contract level and none are to be seen today).
Additionally, the front end also shares the responsibility to address this issue.
Here the rules provided in the sample deployment are definitively not good, and if those were to be in scope, the issue would have been valid. The thing is that those were not finished (didn't handle markets others than categorical), this was known and specified in an exclusion: "Issues about the rules (Reality and Curate) not being up to date in the sample deployment (we provided a sample deployment to allow hunter to get an idea of how those will be deployed and test on them, but the exact rule documents are still being finalized)."
Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0x24e53ce310ba03cf69c8d0d4e6713fbbdcfb328a14a244fbe871fe7000ce751e Severity: high
Description:
description
The user could make a valid scalar market with upperBound lower than
type(uint256).max - 2
. the valid answer could be between upperBound and lowerBound. if the answer will be between upperBound and lowerBound, the result will beupperBound - answer
andanswer - lowerBound
. so if payout will be large number it will create overflow in redemption processScenario
If a market is created with an
upperBound
of2^256 - 3
,lowerBound
of 1 and the resolved answer is10
, the resulting payouts will be:upperBound - answer = 2^256 - 3 - 10
answer - lowerBound = 10 - 1
When a user's token balance is multiplied by the payout values, this large difference can overflow, causing the redemption process to fail.
impact
Users may not be able to redeem their tokens due to overflow during the redemption calculation.
POC:
A test case demonstrating the issue:
The logs show the test fails due to a multiplication overflow:
line of issue
https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/blob/4e56254cbd071b6f678a108ccdb8660951636d27/contracts/src/RealityProxy.sol#L145-L147
https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/blob/4e56254cbd071b6f678a108ccdb8660951636d27/contracts/src/interaction/conditional-tokens/ConditionalTokens.sol#L250
https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/blob/4e56254cbd071b6f678a108ccdb8660951636d27/contracts/src/MarketFactory.sol#L187
Mitigation
Check the upperBound in the
createScalarMarket
to be less thantype(uint128).max
.