Users unable to redeem due to overflow

[hats-bug-reporter[bot]]

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0xde135ac616f16a3c0c7f1c8f0525bf4331326803b0681e00426aad0be2cae995 Severity: high

Description:

description

The user could make a scalar market with upper lower than type256max, and if the answer will be between min and max the result will be max - answer

Consider a scenario where a user creates a valid market with 2^256 - 3. The result is 10, so the output will be 2^256 - 3 - 10. If the user's balance multiplied by the result exceeds the limit, the user can't redeem their tokens.

impact

User unable to redeem their tokens

mitigation

Check upper bond in the factory to be less than type128 max

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

[greenlucid]

I didn't test, but it seems like you're right. In the example you gave, it would already overflow during reportPayouts in this step:

https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/blob/4e56254cbd071b6f678a108ccdb8660951636d27/contracts/src/interaction/conditional-tokens/ConditionalTokens.sol#L97

Interesting how they implemented this safety mechanism for multi scalar markets, but not in this one, was it a slip?:

https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/blob/4e56254cbd071b6f678a108ccdb8660951636d27/contracts/src/RealityProxy.sol#L170

The conditions to trigger this vulnerability seem difficult, though:

• Assuming tokens are collateralized with DAI, 18 decimals, and something grand like 100 million USD, that fits in 87 bits.
• upperBound must be something big, within the order of 1e50. Or, anything that makes upperBound * biggestBetInWei be around 1e76.
• Users are somehow comfortable with playing in a market with such a monstruous range, and the frontend somehow allows for a comfortable experience entering those numbers. (This is more dubious by the fact that, markets with such big ranges, might be better suited to have a logaritmic answer.)
• Upon someone creating such a market with such exotic parameters, no one in the team will realize they implemented a failsafe in a very similar case, and warn before it gets popular and there are funds at risk.

I wouldn't call this a high vulnerability (it seems very theoretical, I doubt scalar markets with upperBound > 1e9 even get any traction, let alone something like 50 digits), but I think it's indeed something Seer might agree it's an issue, since they addressed it for a similar case.

Out of Scope

Any issue that is only theoretical but can't happen in practice.

[0xmahdirostami]

@greenlucid thanks for your feedback, actually i resubmitted it in issue #100 with more details to become eligible for a high-quality report, i think its better to discuss under that issue.

For now, I want to give feedback on your comment:

I didn't test, but it seems like you're right. In the example you gave

• the test is provided in issue 100

It would already overflow during reportPayouts in this step: https://github.com/hats-finance/SeeR-PM-0x899bc13919880db76edf4ccd72bdfa5dfa666fb7/blob/4e56254cbd071b6f678a108ccdb8660951636d27/contracts/src/interaction/conditional-tokens/ConditionalTokens.sol#L97

• actually not and the overflow will happen in another line that I mention in the report

Interesting how they implemented this safety mechanism for multi-scalar markets

• exactly true

The conditions to trigger this vulnerability seem difficult, though: I wouldn't call this a high vulnerability (it seems very theoretical, I doubt scalar markets with upperBound > 1e9 even get any traction, let alone something like 50 digits), but I think it's indeed something Seer might agree it's an issue, since they addressed it for a similar case.

• The market is valid, and the answer is valid, as I mentioned in the other report. So the contract must handle these valid markets and valid answers.
• Respectively, there is no fact-based statement, and it's just a guess. Due to the scope of competition, if the user is unable to redeem, it's a high-severity issue.

Long-term freezing of user funds (ex: preventing a market from resolving for 100 years) or Significant protocol insolvency.

In this issue, user is unable to redeem for 100 years.

[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.

[greenlucid]

Another point that can be made here:

• 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.