code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/libraries/Twa.sol#L11 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Pool.sol#L121 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Pool.sol#L294

Vulnerability details

Impact

TWA Price should be updated in addLiquidity, removeLiqudity and swap and transferLiquidity

Proof of Concept

I want to quote from the documentation:

https://medium.com/maverick-protocol/maverick-amm-the-revolutionary-amm-that-enables-directional-lping-unlocking-greater-capital-34427f5ac22f

In Maverick, the AMM smart contract tracks the time-weighted average price (TWAP) with a configurable lookback period (the default choice is a 3 hour lookback, but anyone can start a pool with an arbitrary lookback). As trades happen, the price–and thus the TWAP–changes. When the TWAP moves through a bin edge up, Mode Right and Mode Both bins will move up to the next bin too. Likewise, as the TWAP moves down through a bin edge, Mode Left and Mode Both bins will shift down one bin.

The Twa is crucial component in the protocol,

In the current implementation of the Pool, the twa is only updated when the first user addLiquidity or the swap happens.

However, when underlying liqudity change, the TWAP should be updated, otherwise, a invalid or stale price can be used.

THe operation addLiquidity, removeLiqudity and swap and migrateBinsUpStack and transferLiquidity change underlying liquditiy, so in each operation,

function updateValue(IPool.TwaState storage self, int256 value) internal {
    self.twa = self.lastTimestamp == 0 ? int96(value) : int96(getTwa(self));
    self.lastTimestamp = uint48(block.timestamp);
    self.value = int96(value);
}

twa.updateValue should be called.

For example, when swap, the price used is:

int32 startingTick = currentState.activeTick;
int32 lastTwa = twa.floor();

We does not check if the twa.floor() price is stale or if it is updated after adding or remove liqudity.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project update the TWAP price when a operation change the underlying liqudity.

gte620v commented 1 year ago

Disagree. We only need to update the twap when the price of the AMM changes. Price changes only occur during a swap, so we only need to update twap on a swap. The twap does not get "stale". As time passes, the twap accounts for this time passing as part of its weighting math.

c4-sponsor commented 1 year ago

gte620v marked the issue as sponsor disputed

kirk-baird commented 1 year ago

This issue is invalid. TWAP stands for Time Weighted Average Price and should only be updated when there are changes in price. Excluding the first add liquidity which sets the price, other movements of liquidity do not effect price.

c4-judge commented 1 year ago

kirk-baird marked the issue as unsatisfactory: Invalid

code-423n4 / 2022-12-Stealth-Project-findings