code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L124-L127 https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/GeVault.sol#L145-L148

Vulnerability details

Impact

In both pushTick and shiftTick functions, there is a check to ensure that the new tick being added is properly ordered in ascending price order relative to the existing ticks in the ticks array. However, the condition for this check is incorrect. In the pushTick function, the condition checks if baseTokenIsToken0, and in the shiftTick function, it checks for !baseTokenIsToken0. The baseTokenIsToken0 variable determines whether the base token is token0 or token1. However, the conditions should be the opposite. The incorrect conditions can lead to improper tick ordering, which might cause unintended consequences during the contract's execution.

Proof of Concept

In both pushTick and shiftTick functions, the check for properly ordered ticks seems to be incorrect. The condition for the pushTick function checks if the baseTokenIsToken0, and the condition for the shiftTick function checks if !baseTokenIsToken0, but it should be the opposite. The correct conditions should be reversed to ensure the properly ordered ticks.

In pushTick

if (baseTokenIsToken0)
    require(t.lowerTick() > ticks[ticks.length-1].upperTick(), "GEV: Push Tick Overlap");
else
    require(t.upperTick() < ticks[ticks.length-1].lowerTick(), "GEV: Push Tick Overlap");

In shiftTick

if (!baseTokenIsToken0)
    require(t.lowerTick() > ticks[0].upperTick(), "GEV: Shift Tick Overlap");
else
    require(t.upperTick() < ticks[0].lowerTick(), "GEV: Shift Tick Overlap");

The issue is that the conditions are opposite of what they should be. In the pushTick function, it should check !baseTokenIsToken0 instead of baseTokenIsToken0, and in the shiftTick function, it should check baseTokenIsToken0 instead of !baseTokenIsToken0. This will ensure that the ticks are properly ordered in the ascending price order, as intended.

Let's create Something.

Suppose we have a GeVault smart contract deployed with the baseTokenIsToken0 set to true. In this scenario, the base token is token0, and we want to push a new tick with a lower tick value than the last tick in the ticks array.

Here is the initial state of the ticks array.

ticks: [tick1, tick2, tick3]

Where tick1 has a lower tick value than tick2, and tick2 has a lower tick value than tick3.

Now, we try to push a new tick, tick4, with a lower tick value than the last tick in the array (tick3). According to the incorrect check in the pushTick function, it should allow pushing tick4 because baseTokenIsToken0 is set to true, and the condition checks for t.lowerTick() > ticks[ticks.length-1].upperTick(), which translates to tick4.lowerTick() > tick3.upperTick(). Since tick4 has a lower tick value than tick3, the condition is satisfied, and tick4 is pushed into the ticks array.

After the push, the state of the ticks array becomes.

ticks: [tick1, tick2, tick3, tick4]

However, this is incorrect because the ticks are no longer properly ordered in ascending price order.

Tools Used

VsCode

Recommended Mitigation Steps

In pushTick:

if (!baseTokenIsToken0)
    require(t.upperTick() < ticks[ticks.length-1].lowerTick(), "GEV: Push Tick Overlap");
else
    require(t.lowerTick() > ticks[ticks.length-1].upperTick(), "GEV: Push Tick Overlap");

In shiftTick:

if (baseTokenIsToken0)
    require(t.upperTick() < ticks[0].lowerTick(), "GEV: Shift Tick Overlap");
else
    require(t.lowerTick() > ticks[0].upperTick(), "GEV: Shift Tick Overlap");

This will ensure that the ticks are properly ordered in the ascending price order, as intended.

Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

141345 marked the issue as duplicate of #125

c4-judge commented 1 year ago

gzeon-c4 changed the severity to QA (Quality Assurance)