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#L126

Vulnerability details

Impact

Liquidity can be biased on a specific side (quote vs base) and it is even possible a liquidity provider gets more LP tokens.

Proof of Concept

According to the PDF document provided, the number of LP tokens newSupply is calculated using the Table 1 as below. Imgur

In the document, reserveA and reserveB represent the total amount of the quote token and the base token in a specific price range $[p_l, p_u]$ respectively.

In the implementation, the protocol gets the first valid bin and use its reserve amounts for this.

// Pool.sol
113:     function addLiquidity(
114:         uint256 tokenId,
115:         AddLiquidityParams[] calldata params,
116:         bytes calldata data
117:     ) external override returns (uint256 tokenAAmount, uint256 tokenBAmount, BinDelta[] memory binDeltas) {
118:         State memory currentState = checkReentrancy(false, true);
119:         // ************************ reentrancy ************************
120:         if (twa.lastTimestamp == 0) {
121:             twa.updateValue(currentState.activeTick * PRBMathSD59x18.SCALE);
122:         }
123:         binDeltas = new BinDelta[](params.length);
124:
125:         AddLiquidityTemp memory temp;
126:         (temp.reserveA, temp.reserveB) = _firstKindAtTickLiquidity(currentState.activeTick);//@audit should use the aggregated reserve
127:         for (uint256 i; i < params.length; i++) {
128:             AddLiquidityParams memory param = params[i];
129:             require(param.kind < NUMBER_OF_KINDS);
130:
131:             (uint128 binId, Bin.Instance storage bin) = _getOrCreateBin(currentState, param.kind, param.pos, param.isDelta);
132:
133:             (temp.deltaA, temp.deltaB, temp.deltaLpBalance) = bin.addLiquidity(
134:                 tokenId,
135:                 Math.fromScale(param.deltaA, tokenAScale),//@audit-info deltaA is what the user specified, that's why we do scaling
136:                 Math.fromScale(param.deltaB, tokenBScale),
137:                 currentState.activeTick,
138:                 temp.reserveA,
139:                 temp.reserveB
140:             );
...

The function _firstKindAtTickLiquidity() finds a bin of any kind that is registered.

// Pool.sol
537:     function _firstKindAtTickLiquidity(int32 activeTick) internal view returns (uint256 currentReserveA, uint256 currentReserveB) {
538:         uint256 word = binMap.getKindsAtTick(activeTick);
539:
540:         for (uint256 i; i < NUMBER_OF_KINDS; i++) {
541:             if ((word & (1 << i)) != 0) {
542:                 uint128 binId = binPositions[activeTick][i];
543:                 Bin.Instance storage bin = bins[binId];
544:                 return (bin.state.reserveA, bin.state.reserveB);
545:             }
546:         }
547:     }

And the kinds of bins are defined as constants to be used in bit operations.

Constants.sol
8: uint8 constant KIND_STATIC = 1;
9: uint8 constant KIND_RIGHT = 2;
10: uint8 constant KIND_LEFT = 4;
11: uint8 constant KIND_BOTH = 8;

This can lead to a few possible problems in some scenarios. First, the liquidity can be biased into a specific side (base or quote). As we can see from the liquidity calculation table, if either reserveA or reserveB is zero, the user deposit is limited to one side (A or B). So this results in unnecessary bias in the liquidity. Note that if we use the aggregated reserve values, it will be better. For example, let's say there is a bin of KIND_RIGHT with reserveA>0, reserveB=0. Then all deposits for this price range, only quote tokens will be deposited until it is not merged. Second, more LP tokens might be minted. From the table 1, we can see that if reseveA>0, reserveB>0, a minimum of two side values are used as an amount of LP tokens. So in a scenario same to the first case, the user will continue getting more LP tokens because only one side is used when either one side's reserve amount is zero.

From the conversation with the sponsor, it is understood that it is gassier (but still limited to 4 loops) to use aggregated values but I believe this can lead to a problem that costs more than the gas concern.

Tools Used

Manual Review

Recommended Mitigation Steps

Use the aggregated amounts of all kinds of bins instead of one bin as the reserve amounts while adding liquidity.

gte620v commented 1 year ago

All the bins in a given tick have the same reserve ratio. This is enforced by these calculations https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/libraries/Bin.sol#L42-L48. So it is fine to use the ratio of the first bin as this will be the same as the ratio of the aggregate.

Moreover, a user has the additional protections discussed in https://github.com/code-423n4/2022-12-Stealth-Project-findings/issues/92#issuecomment-1354018166 to avoid an add-liquidity situation that they do not expect.

c4-sponsor commented 1 year ago

gte620v marked the issue as sponsor disputed

kirk-baird commented 1 year ago

As all bins have the same reserve ratio at a tick, it is indifferent to use one bin vs the aggregate of all bins.

I'm going to mark this issue as invalid.

c4-judge commented 1 year ago

kirk-baird marked the issue as nullified

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