sherlock-admin2 commented 7 months ago

xiaoming90

medium

AMM will revert if exchange rate is one

Summary

The AMM will stop working unexpectedly when the preTradeExchangeRate is 1.0.

Vulnerability Detail

https://github.com/sherlock-audit/2024-01-napier/blob/main/v1-pool/src/libs/PoolMath.sol#L213

File: PoolMath.sol
203:     function _getRateAnchor(
204:         uint256 totalBaseLptTimesN,
205:         uint256 lastLnImpliedRate,
206:         uint256 totalUnderlying18,
207:         int256 rateScalar,
208:         uint256 timeToExpiry
209:     ) internal pure returns (int256 rateAnchor) {
210:         // `extRate(t*) = e^(lastLnImpliedRate * yearsToExpiry(t))`
211:         // Get pre-trade exchange rate with zero-fee
212:         int256 preTradeExchangeRate = _getExchangeRateFromImpliedRate(lastLnImpliedRate, timeToExpiry);
..SNIP..
213:         // exchangeRate should not be below 1.
214:         // But it is mathematically almost impossible to happen because `exp(x) < 1` is satisfied for all `x < 0`.
215:         // Here x = lastLnImpliedRate * yearsToExpiry(t), which is very unlikely to be negative.(or
216:         // more accurately the natural log rounds down to zero). `lastLnImpliedRate` is guaranteed to be positive when it is set
217:         // and `yearsToExpiry(t)` is guaranteed to be positive because swap can only happen before maturity.
218:         // We still check for this case to be safe.
219:         require(preTradeExchangeRate > SignedMath.WAD);
220:         uint256 proportion = totalBaseLptTimesN.divWadDown(totalBaseLptTimesN + totalUnderlying18);
221:         int256 lnProportion = _logProportion(proportion);
222: 
223:         // Compute `rateAnchor(t) = extRate(t*) - ln(portion(t*)) / rateScalar(t)`
224:         rateAnchor = preTradeExchangeRate - lnProportion.divWadDown(rateScalar);

At Line 219, it requires that the preTradeExchangeRate be larger than 1.0. However, technically, the exchange rate can be 1.0 based on the comment in Line 213 that the exchange rate should not be below one, which means that the exchange rate should be 1.0 or above. Thus, when the preTradeExchangeRate is 1.0, the AMM will revert unexpectedly.

Impact

The AMM might stop working unexpectedly. Breaking of core protocol/contract functionality.

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/v1-pool/src/libs/PoolMath.sol#L213

Tool used

Manual Review

Recommendation

Consider making the following change:

function _getRateAnchor(
    uint256 totalBaseLptTimesN,
    uint256 lastLnImpliedRate,
    uint256 totalUnderlying18,
    int256 rateScalar,
    uint256 timeToExpiry
) internal pure returns (int256 rateAnchor) {
    ..SNIP..
-    require(preTradeExchangeRate > SignedMath.WAD);
+    require(preTradeExchangeRate >= SignedMath.WAD);

Sidenote: This is also implemented in Pendle's Math Library

massun-onibakuchi commented 7 months ago

@ydspa

I think this severity can be informational. I agree that the recommended change would be better for safety.

The AMM will stop working unexpectedly when the preTradeExchangeRate is 1.0.

I think the suggested issue never happens.

At Line 219, it requires that the preTradeExchangeRate be larger than 1.0. However, technically, the exchange rate can be 1.0 based on the comment in Line 213 that the exchange rate should not be below one, which means that the exchange rate should be 1.0 or above. Thus, when the preTradeExchangeRate is 1.0, the AMM will revert unexpectedly.

For the preTradeExchangeRate to be 1, lastImpliedRate must become zero. However, as you can see from the following snippet, lastImpliedRate never becomes zero.

https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/v1-pool/src/libs/PoolMath.sol#L343-L345

   function _setPostPoolState(
        PoolState memory pool,
        PoolPreCompute memory comp,
        int256 netBaseLptToAccount,
        int256 netUnderlyingToAccount18,
        int256 netUnderlyingToProtocol18
    ) internal view {
        .....
        if (pool.lastLnImpliedRate == 0) revert Errors.PoolZeroLnImpliedRate();
}

This zero check is seen in Pendle as well.

Every swaps, lastImpliedRate is updated and used in the next swap. https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/v1-pool/src/libs/PoolMath.sol#L176

    function computeAmmParameters(PoolState memory pool) internal view returns (PoolPreCompute memory cache) {
        uint256 timeToExpiry = pool.maturity - block.timestamp;

        cache.rateScalar = _getRateScalar(pool, timeToExpiry);
        cache.rateAnchor = _getRateAnchor(
            pool.totalBaseLptTimesN, pool.lastLnImpliedRate, pool.totalUnderlying18, cache.rateScalar, timeToExpiry
        );
        cache.feeRate = _getExchangeRateFromImpliedRate(pool.lnFeeRateRoot, timeToExpiry);
    }

https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/v1-pool/src/libs/PoolMath.sol#L165C9-L165C26

    function executeSwap(PoolState memory pool, int256 netBaseLptToAccount)
        internal
        view
        returns (int256 netUnderlyingToAccount18, int256 netUnderlyingFee18, int256 netUnderlyingToProtocol18)
    {
        ....
        _setPostPoolState(pool, comp, netBaseLptToAccount, netUnderlyingToAccount18, netUnderlyingToProtocol18);
    }

Banditx0x commented 7 months ago

Escalate

on behalf of sponsor. Note: I have not read through issue myself. @cvetanovv @massun-onibakuchi @Czar102

sherlock-admin2 commented 7 months ago

Escalate

on behalf of sponsor. Note: I have not read through issue myself. @cvetanovv @massun-onibakuchi @Czar102

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

cvetanovv commented 7 months ago

I agree with the escalation and the comment of the sponsor.

sherlock-admin4 commented 6 months ago

The protocol team fixed this issue in PR/commit https://github.com/napierfi/v1-pool/pull/155.

Czar102 commented 6 months ago

In that case, planning to accept the escalation and invalidate the issue.

Czar102 commented 6 months ago

Result: Invalid Unique

sherlock-admin3 commented 6 months ago

Escalations have been resolved successfully!

Escalation status:

banditx0x: accepted