Maverick contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

Pool._amountToBin() returns a larger value than it should when protocolFeeRatio = 100%.

As a result, bin balances might be calculated wrongly.

Proof of Concept

delta.deltaInBinInternal is used to update the bin balances like this.

    if (tokenAIn) {
        binBalanceA += delta.deltaInBinInternal.toUint128();
        binBalanceB = Math.clip128(binBalanceB, delta.deltaOutErc.toUint128());
    } else {
        binBalanceB += delta.deltaInBinInternal.toUint128();
        binBalanceA = Math.clip128(binBalanceA, delta.deltaOutErc.toUint128());
    }

As we can see here, _amountToBin() is used to calculate delta.deltaInBinInternal from deltaInErc and feeBasis.

    uint256 feeBasis = Math.mulDiv(binAmountIn, fee, PRBMathUD60x18.SCALE - fee, true);
    delta.deltaInErc = binAmountIn + feeBasis;
    delta.deltaInBinInternal = _amountToBin(delta.deltaInErc, feeBasis);
    delta.excess = swapped ? Math.clip(amountOut, delta.deltaOutErc) : 0;

With the above code, the protocol fee should be a portion of feeBasis and it is the same as feeBasis when state.protocolFeeRatio = ONE_3_DECIMAL_SCALE.

But when we check _amountToBin(), the actual protocol fee will be feeBasis + 1 and delta.deltaInBinInternal will be less than its possible smallest value(= binAmountIn).

    function _amountToBin(uint256 deltaInErc, uint256 feeBasis) internal view returns (uint256 amount) { 
        amount = state.protocolFeeRatio != 0 ? Math.clip(deltaInErc, feeBasis.mul(uint256(state.protocolFeeRatio) * PROTOCOL_FEE_SCALE) + 1) : deltaInErc;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

We should modify _amountToBin() like below.

    function _amountToBin(uint256 deltaInErc, uint256 feeBasis) internal view returns (uint256 amount) { 
        if (state.protocolFeeRatio == ONE_3_DECIMAL_SCALE)
            return deltaInErc - feeBasis;

        amount = state.protocolFeeRatio != 0 ? Math.clip(deltaInErc, feeBasis.mul(uint256(state.protocolFeeRatio) * PROTOCOL_FEE_SCALE) + 1) : deltaInErc;
    }

Lines of code

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

Vulnerability details

Impact

Time-warped-price is updated incorrectly and this affects moving bins.

Proof of Concept

The protocol updates twa on every swap and uses that to decide how to move bins. But in the function swap(), the delta's endSqrtPrice can not contribute negatively to the activeTick because it is wrapped with a clip() function.

// Pool.sol
286:         if (amountOut != 0) {
287:             if (tokenAIn) {
288:                 binBalanceA += delta.deltaInBinInternal.toUint128();
289:                 binBalanceB = Math.clip128(binBalanceB, delta.deltaOutErc.toUint128());
290:             } else {
291:                 binBalanceB += delta.deltaInBinInternal.toUint128();
292:                 binBalanceA = Math.clip128(binBalanceA, delta.deltaOutErc.toUint128());
293:             }
294:             twa.updateValue(currentState.activeTick * PRBMathSD59x18.SCALE + int256(Math.clip(delta.endSqrtPrice, delta.sqrtLowerTickPrice).div(delta.sqrtUpperTickPrice - delta.sqrtLowerTickPrice)));//@audit second part of the sum is always positive, should contribute both ways
295:         }
296:

I believe delta.endSqrtPrice should contribute both ways to the activeTick and it is quite possible for the endSqrtPrice to be out of the range (delta.sqrtLowerTickPrice, delta.sqrtUpperTickPrice). (In another report, I mentioned an issue of accuracy loss in the calculation of endSqrtPrice).

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend changing the relevant line as below without using clip() so that the endSqrtPrice can contribute to the twa reasonably.

294:             twa.updateValue(currentState.activeTick * PRBMathSD59x18.SCALE + int256((delta.endSqrtPrice, delta.sqrtLowerTickPrice).div(delta.sqrtUpperTickPrice - delta.sqrtLowerTickPrice)));

Judge has assessed an item in Issue #100 as M risk. The relevant finding follows:

Lines of code https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/libraries/Bin.sol#L35 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/libraries/Bin.sol#L38

Vulnerability details Impact The wrong amount of LP tokens will be minted and the wrong amount of A/B tokens will be deposited.

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

This is implemented in the function lpTokensFromDeltaReserve() of Bin.sol as belows.

function lpTokensFromDeltaReserve(
    Bin.Instance storage self,
    uint256 _deltaA,
    uint256 _deltaB,
    int32 _activeLowerTick,
    uint256 _existingReserveA,
    uint256 _existingReserveB
) internal view returns (uint256 deltaAOptimal, uint256 deltaBOptimal, uint256 proRataLiquidity) {
    deltaAOptimal = _deltaA;
    deltaBOptimal = _deltaB;
    bool noA = self.state.reserveA == 0;
    bool noB = self.state.reserveB == 0;

    if (self.state.lowerTick < _activeLowerTick || (!noA && noB)) {//@audit case p<p_l, the inequality should be opposite side
        deltaBOptimal = 0;
        proRataLiquidity = noA || self.state.totalSupply == 0 ? deltaAOptimal : Math.mulDiv(deltaAOptimal, self.state.totalSupply, self.state.reserveA, false);
    } else if (self.state.lowerTick > _activeLowerTick || (noA && !noB)) {//@audit case p>p_u, the inequality should be opposite side
        deltaAOptimal = 0;
        proRataLiquidity = noB || self.state.totalSupply == 0 ? deltaBOptimal : Math.mulDiv(deltaBOptimal, self.state.totalSupply, self.state.reserveB, false);
    } else {
        if (_existingReserveA > 0) {
            deltaBOptimal = Math.mulDiv(_existingReserveB, _deltaA, _existingReserveA, false);
        }
        if (deltaBOptimal > _deltaB && _existingReserveB > 0) {
            deltaAOptimal = Math.mulDiv(_existingReserveA, _deltaB, _existingReserveB, false);
            deltaBOptimal = _deltaB;
        }

        proRataLiquidity = (noA && noB) || self.state.totalSupply == 0
            ? Math.max(deltaAOptimal, deltaBOptimal)
            : Math.min(Math.mulDiv(deltaAOptimal, self.state.totalSupply, self.state.reserveA, false), Math.mulDiv(deltaBOptimal, self.state.totalSupply, self.state.reserveB, false));
    }
}

The implementation uses wrong inequalities for the edge cases $p\lt p_l$ and $p\gt p_u$.

Tools Used Manual Review

Recommended Mitigation Steps Reverse the inequalities at Bin.sol#L35 and Bin.sol#L38.

[L-01] Pool.checkPositionAccess() doesn't check the approvedForAll users.

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

    function checkPositionAccess(uint256 tokenId) internal view {
        require(msg.sender == position.ownerOf(tokenId) || msg.sender == position.getApproved(tokenId), "P");
    }

Currently, it checks only the owner or approved user for the tokenId for the access permission.

But according to the openzeppelin implement, it should allow isApprovedForAll users as well because such users can be the token owner without any requirements by transferring the token.

    function _isApprovedOrOwner(address spender, uint256 tokenId) internal view virtual returns (bool) {
        address owner = ERC721.ownerOf(tokenId);
        return (spender == owner || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender);
    }

[L-02] Math.mulDiv() returns a wrong value when ceil = true

• https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/libraries/Math.sol#L35

    function mulDiv(uint256 x, uint256 y, uint256 k, bool ceil) internal pure returns (uint256) {
        if (y == k) return x;
        return ceil ? PRBMath.mulDiv(x, y, k) + 1 : PRBMath.mulDiv(x, y, k);
    }

Currently, ceilVal = floorVal + 1 all the time but they should be same when PRBMath.mulDiv(x, y, k) is an exact uint value without roundings.

[L-03] Pool.setProtocolFeeRatio() should have a upper limit.

• https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Pool.sol#L332
• https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Factory.sol#L26-L38

When we set a protocolFeeRatio, it can be ONE_3_DECIMAL_SCALE which means 100% of the pool fee.

Then all amounts of feeBasis will be a protocol fee here, no benefit for liquidity providers.

[L-04] Check address(0) for recipient

• https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Pool.sol#L260
• https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Pool.sol#L209
• https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Pool.sol#L339

When users or admin set the recipient, there is no validation of address(0) so the funds might be lost.

[N-01] Typo

• https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/interfaces/IPool.sol#L60

"amount of B token in bin" => "amount of A token in bin"