code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/libraries/BinMap.sol#L26 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/libraries/BinMap.sol#L31 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/libraries/BinMap.sol#L36 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/libraries/BinMap.sol#L42 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/libraries/BinMap.sol#L78 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Pool.sol#L538 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Pool.sol#L620

Vulnerability details

Impact

The BinMap library performs multiplication on int32 values that can potentially overflow and cause the corresponding function calls to revert. The functions in question are used by essential Pool methods such as Pool.addLiquidity or Pool.swap and an overflow would cause a permanent DOS to these functions and as a result also to the corresponding pool instance.

Proof of Concept

The BinMap library performs the following logic in almost all its functions:

getMapPointer((tick * NUMBER_OF_KINDS_32))

The variable tick describes the currently active tick of the calling pool and is an int32 that is passed as function parameter. The value of NUMBER_OF_KINDS_32 is 4 and multiplication with it essentially just performs a leftshift by 2 bits. Opposed to a leftshift operation, a multiplication can overflow though, as showcased in the following simple foundry tests:

function testSignedIntOverflow() public {
    int32 tick = type(int32).max;
    console.log(uint256(uint32(tick)));
    uint8 NUMBER_OF_KINDS = 4;
    int32 NUMBER_OF_KINDS_32 = int32(uint32(NUMBER_OF_KINDS));

    vm.expectRevert();
    int32 result = tick * NUMBER_OF_KINDS_32;

}

function testShiftOverflow() public {
    int32 tick = type(int32).max;

    int32 result = tick << 2; //works
}

The BinMap library is used in the Pool.addLiquidity and Pool.swap early on, e.g. in a call to binMap.getKindsAtTick(activeTick). Those are also the only functions that are able to change the value of Pool.state.activeTick, hence if those two functions break, the pool instance is rendered unusable indefinitely.

A simple example where it is evident that a tick value that high can be actually reached is the following scenario: a malicious actor watches the mempool for calls to Factory.create and frontruns the call with the same data except he replaces the _activeTick param with type(int32).max which will cause state.activeTick = _activeTick right in the constructor of the Pool and cause the DOS described above. Though this kind of griefing can be considered questionable from a cost-perspective, as there is an infinite amount of possible pool configurations, this example serves to highlight the possibility of the issue occuring in practice.

Tools Used

Manual review and foundry.

Recommended Mitigation Steps

Replace all occurrences of tick * NUMBER_OF_KINDS_32 in the BinMap library with tick << 2. Alternatively restrict the maximum tick value in the Pool contract.