code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L167-L173

Vulnerability details

Impact

The modifyTick function is used by the GeVault owner to update a given tick (at a certain index) from the ticks array, but the function does not check if the new updated tick is ordered properly in the array which can result in unexpected behaviours for example when distributing liquidity across the different ticks.

Proof of Concept

The issue occurs in the modifyTick function below :

 function modifyTick(address tr, uint index) public onlyOwner {
    (ERC20 t0, ) = TokenisableRange(tr).TOKEN0();
    (ERC20 t1, ) = TokenisableRange(tr).TOKEN1();
    require(t0 == token0 && t1 == token1, "GEV: Invalid TR");
    // @audit dpesn't verify that tick is properly ordered
    ticks[index] = TokenisableRange(tr);
    emit ModifyTick(tr, index);
}

As we can see the function updates the tick immediately without checking its ordering.

In the contract we can also notice that there are 2 other functions that are responsible for setting new ticks : pushTick and shiftTick and both contain check for the tick order, for example in the case of pushTick we have :

function pushTick(address tr) public onlyOwner {
    TokenisableRange t = TokenisableRange(tr);
    (ERC20 t0, ) = t.TOKEN0();
    (ERC20 t1, ) = t.TOKEN1();
    require(t0 == token0 && t1 == token1, "GEV: Invalid TR");
    if (ticks.length == 0) ticks.push(t);
    else {
        // @audit here is the tick order check
        // Check that tick is properly ordered
        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"
            );

        ticks.push(TokenisableRange(tr));
    }
    emit PushTick(tr);
}

And the same kind of check is implemented in the shiftTick function, because the modifyTick function does not have this check it can allow a tick misplacement which will result in unexpected behaviours for the protocol like when distributing liquidity across the ticks.

** I submit this issue as Medium risk because only the contract owner is able to call the function, but i believe it is still a critical due to its potential implications on the protocol (regardless if the owner is malicious or not), as if an error occurs when modifying a certain tick and this error go unnoticed, it can later impact the actions of other functions like pushTick and shiftTick which will shuffle the order even further .

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this issue i recommend to verify that the new updated tick is correctly ordered in the list, the modifyTick function should have similar checks to the ones present in the functions pushTick and shiftTick.

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)