fibonacci - JalaPair potential permanent DoS due to overflow

[sherlock-admin]

fibonacci

medium

JalaPair potential permanent DoS due to overflow

Summary

In the JalaPair::_update function, overflow is intentionally desired in the calculations for timeElapsed and priceCumulative. This is forked from the UniswapV2 source code, and it’s meant and known to overflow. UniswapV2 was developed using Solidity 0.6.6, where arithmetic operations overflow and underflow by default. However, Jala utilizes Solidity >=0.8.0, where such operations will automatically revert.

Vulnerability Detail

uint32 timeElapsed = blockTimestamp - blockTimestampLast; // overflow is desired
if (timeElapsed > 0 && _reserve0 != 0 && _reserve1 != 0) {
    // * never overflows, and + overflow is desired
    price0CumulativeLast += uint256(UQ112x112.encode(_reserve1).uqdiv(_reserve0)) * timeElapsed;
    price1CumulativeLast += uint256(UQ112x112.encode(_reserve0).uqdiv(_reserve1)) * timeElapsed;
}

Impact

This issue could potentially lead to permanent denial of service for a pool. All the core functionalities such as mint, burn, or swap would be broken. Consequently, all funds would be locked within the contract.

I think issue with High impact and a Low probability (merely due to the extended timeframe for the event's occurrence, it's important to note that this event will occur with 100% probability if the protocol exists at that time), should be considered at least as Medium.

References

There are cases where the same issue is considered High.

https://solodit.xyz/issues/h-02-uniswapv2priceoraclesol-currentcumulativeprices-will-revert-when-pricecumulative-addition-overflow-code4rena-phuture-finance-phuture-finance-contest-git
https://solodit.xyz/issues/m-02-twavsol_gettwav-will-revert-when-timestamp-4294967296-code4rena-nibbl-nibbl-contest-git
https://solodit.xyz/issues/trst-m-3-basev1pair-could-break-because-of-overflow-trust-security-none-satinexchange-markdown_

Code Snippet

https://github.com/sherlock-audit/2024-02-jala-swap/blob/main/jalaswap-dex-contract/contracts/JalaPair.sol#L97-L102

Tool used

Manual Review

Recommendation

Use the unchecked block to ensure everything overflows as expected

[nevillehuang]

Request poc

Would like the watson/watsons to present a scenario where a reasonable overflow can be achieved, because based on my discussion with LSW this is likely not reasonable considering frequency of liquidity addition and ratio of reserves required

#186 - let's put it this way. Ratio is scaled up by 2^112. Having a (ratio * timeElapsed1) + (ratio * timeElapsed2) is the same as having (ratio * (timeElapsed1 + timeElapsed2). So if we have a token1 max reserve of uint112 and token0 reserve is 1, uint112.max * uint112.max * totalTimeElapsed. In order for this to overflow, we need totalTimeElapsed to be > uint32.max, which is approx 132 years. So for this to overflow, we'd need to have the pool running for 132 years with one of the reserve being the max and the other one being just 1 wei for the entirety of the 132 years.

[sherlock-admin3]

PoC requested from @0xf1b0

Requests remaining: 10

[0xf1b0]

Even if we do not take reserves into account, the timestamp is converted into a 32-bit value.

uint32 blockTimestamp = uint32(block.timestamp % 2 ** 32);

When the timestamp gets >4294967296 (in 82 years), the _update function will always revert.

[nevillehuang]

@Czar102 what do you think? I don’t think its severe enough to warrant medium severity. I remember you rejected one of a similar finding in dodo as seen here

[Czar102]

The linked issue presented a way to DoS functionality. Is this also the case here? Or are funds locked here as well?
It seems to me that the funds are locked, so I'd accept this as a valid Medium (High impact in far future – a limitation).

[nevillehuang]

@Czar102 Agree, if long enough, all core functionalities burn() (Remove liquidity), mint() (add liquidity) and swap() that depends on this low level functions have the potential to be DoSed due to _update() reverting, will leave as medium severity

[deadrosesxyz]

Escalate

block.timestamp stored in a uint32 will overflow in year 2102. This is way too far in the future and likely even beyond our lifetime. Even in the unlikely scenario where the protocol will be used in 80 years from now, users will know years in advance that they'll have to withdraw their funds as the protocol is about to shut down. Issue should be considered low/info

[sherlock-admin2]

Escalate

block.timestamp stored in a uint32 will overflow in year 2102. This is way too far in the future and likely even beyond our lifetime. Even in the unlikely scenario where the protocol will be used in 80 years from now, users will know years in advance that they'll have to withdraw their funds as the protocol is about to shut down. Issue should be considered low/info

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.

[nevillehuang]

I will invite any watsons to prove if accumulated actions (swap, burn and mint) can cause overflow in price variables (whether maliciously or accidentally)

[giraffe0x]

Timestamp overflow is less of a concern here, cumulative value is.

1. As with uniswap, pools can be created with all sorts of exotic pairs. So ratio between reserve0/1 may be arbitrarily large.
2. Code comments clearly state that "overflow is desired" when overflow cannot happen in current format.

It's a clear mistake on the devs, which has an easy fix to wrap with unchecked block. Not sure why they won't fix it.

[deadrosesxyz]

@giraffe0x hey, please check the initial message by @nevillehuang on why cumulatives cannot actually overflow.
Reserves are uint112s, so ratio can be max uint112.max : 1.
Also, the comment is not made by the devs, but actually left out from the copying of original univ2 contracts

[nevillehuang]

@deadrosesxyz, Could a malicious user(s) (despite requiring alot of funds) be able to brick the pool permanently by constantly performing actions (burn, swap, mint) and incrementing price variables?

[deadrosesxyz]

@nevillehuang No, constantly performing actions will not help for an overflow to occur in any way (as the cumulative increases based on seconds passed). Even in the most extreme scenario where one of the reserves has max value and the other one has simply 1 wei, it would take 132 years for an overflow to occur. Quoting my comments from earlier:

let's put it this way. Ratio is scaled up by 2^112. Having a (ratio * timeElapsed1) + (ratio * timeElapsed2) is the same as having (ratio * (timeElapsed1 + timeElapsed2). So if we have a token1 max reserve of uint112 and token0 reserve is 1, uint112.max * uint112.max * totalTimeElapsed. In order for this to overflow, we need totalTimeElapsed to be > uint32.max, which is approx 132 years. So for this to overflow, we'd need to have the pool running for 132 years with one of the reserve being the max and the other one being just 1 wei for the entirety of the 132 years.

[nevillehuang]

This would depend on how @Czar102 interprets the following rule given he agreed initially that this should remain as a valid medium severity as seen here

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

[mahmudoloyede]

@giraffe0x hey, please check the initial message by @nevillehuang on why cumulatives cannot actually overflow. Reserves are uint112s, so ratio can be max uint112.max : 1. Also, the comment is not made by the devs, but actually left out from the copying of original univ2 contracts

It really doesn't matter if it was left out from the copying of original univ2 contracts. The intent is for it to overflow but it won't in this case. Also if the pairs are tokens with decimals greater than 18, the DOS will occur sooner.

[santiellena]

@nevillehuang No, constantly performing actions will not help for an overflow to occur in any way (as the cumulative increases based on seconds passed). Even in the most extreme scenario where one of the reserves has max value and the other one has simply 1 wei, it would take 132 years for an overflow to occur. Quoting my comments from earlier:

let's put it this way. Ratio is scaled up by 2^112. Having a (ratio * timeElapsed1) + (ratio * timeElapsed2) is the same as having (ratio * (timeElapsed1 + timeElapsed2). So if we have a token1 max reserve of uint112 and token0 reserve is 1, uint112.max * uint112.max * totalTimeElapsed. In order for this to overflow, we need totalTimeElapsed to be > uint32.max, which is approx 132 years. So for this to overflow, we'd need to have the pool running for 132 years with one of the reserve being the max and the other one being just 1 wei for the entirety of the 132 years.

@deadrosesxyz It will actually take less than 132 years because the pair could have a token1 max reserve of uint112 and token0 reserve of 1 wei on the first block. This means that timeElapsed will be the block.timestamp of the first block (something like 1710940084 now). This means that 52.25 years of those 132 years have already passed. It is still a big number, however, it is easy to fix and a clear mistake on the devs.

[Czar102]

I maintain my judgment from preliminary judging – I think it should be Medium especially because of the "overflow is desired" comment, which means that sponsors care about the behavior in far future. Without these comments, the judgment on this issue would be disputable.

Planning to reject the escalation and leave the issue as is.

[Czar102]

Result:
Medium
Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• deadrosesxyz: rejected