code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Pool.sol#L453 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Pool.sol#L470

Vulnerability details

Impact

Calculation of startingTick is wrong and unnecessary bins are moved (and possibly merged). This results in unnecessary stacking of bins and the protocol can suffer DOS later.

Proof of Concept

According to the PDF document provided, the bins are moved according to the rules as below. Imgur

This is implemented in the function _moveBins() of Pool.sol as belows.

// Pool.sol
448:     function _moveBins(int32 activeTick, int32 startingTickInitial, int32 lastTwap) internal {
449:         int32 floorTwap = twa.getTwaFloor();
450:
451:         if (activeTick > startingTickInitial || floorTwap > lastTwap) {
452:             MoveData memory moveData;
453:             int32 startingTick = int32(Math.min(startingTickInitial, lastTwap));//@audit min(startingTickInitial-1, lastTwap)
454:
455:             moveData.tickLimit = int32(Math.min(activeTick - 1, floorTwap));//@audit-info target_right
456:
457:             moveData.kinds = [1, 3];
458:             if (startingTick - 1 < moveData.tickLimit) {
459:                 (moveData.activeList, moveData.binCounter) = binMap.getKindsAtTickRange(
460:                     startingTick - 1,
461:                     moveData.tickLimit,
462:                     0x1111111111111111111111111111111111111111111111111111111111111111 * (KIND_RIGHT | KIND_BOTH)
463:                 );
464:                 _moveDirection(activeTick, moveData);
465:             }
466:         }
467:
468:         if (activeTick < startingTickInitial || floorTwap < lastTwap) {
469:             MoveData memory moveData;
470:             int32 startingTick = int32(Math.max(startingTickInitial, lastTwap));//@audit max(startingTickInitial+1, lastTwap)
471:
472:             moveData.tickLimit = int32(Math.max(floorTwap, activeTick + 1));//@audit-info target_left
473:             moveData.kinds = [2, 3];
474:
475:             if (moveData.tickLimit + 1 < startingTick + 2) {
476:                 (moveData.activeList, moveData.binCounter) = binMap.getKindsAtTickRange(
477:                     moveData.tickLimit + 1,
478:                     startingTick + 2,
479:                     0x1111111111111111111111111111111111111111111111111111111111111111 * (KIND_LEFT | KIND_BOTH)
480:                 );
481:                 _moveDirection(activeTick, moveData);
482:             }
483:         }
484:     }

The implementation is calculating starting ticks inconsistent to the document. For example, for the first case the correct starting tick is supposed to be calculated as min(startingTickInitial - 1, lastTwap) but it's implemented as min(startingTickInitial, lastTwap) - 1 (actual starting tick from L#460). Because $min(a,b)-1=min(a-1,b-1)<min(a-1,b)$, this results in moving an unnecessary bin. The similar glitch is on the line L#470 as well.

Tools Used

Manual Review

Recommended Mitigation Steps

Fix as below.

    function _moveBins(int32 activeTick, int32 startingTickInitial, int32 lastTwap) internal {
        int32 floorTwap = twa.getTwaFloor();

        if (activeTick > startingTickInitial || floorTwap > lastTwap) {
            MoveData memory moveData;
            int32 startingTick = int32(Math.min(startingTickInitial - 1, lastTwap));

            moveData.tickLimit = int32(Math.min(activeTick - 1, floorTwap));

            moveData.kinds = [1, 3];
            if (startingTick < moveData.tickLimit) {
                (moveData.activeList, moveData.binCounter) = binMap.getKindsAtTickRange(
                    startingTick,
                    moveData.tickLimit,
                    0x1111111111111111111111111111111111111111111111111111111111111111 * (KIND_RIGHT | KIND_BOTH)
                );
                _moveDirection(activeTick, moveData);
            }
        }

        if (activeTick < startingTickInitial || floorTwap < lastTwap) {
            MoveData memory moveData;
            int32 startingTick = int32(Math.max(startingTickInitial + 1, lastTwap));

            moveData.tickLimit = int32(Math.max(floorTwap, activeTick + 1));
            moveData.kinds = [2, 3];

            if (moveData.tickLimit + 1 < startingTick + 1) {
                (moveData.activeList, moveData.binCounter) = binMap.getKindsAtTickRange(
                    moveData.tickLimit + 1,
                    startingTick + 1,
                    0x1111111111111111111111111111111111111111111111111111111111111111 * (KIND_LEFT | KIND_BOTH)
                );
                _moveDirection(activeTick, moveData);
            }
        }
    }