sherlock-admin2 commented 7 months ago

KingNFT

high

The last user can't quit `Tranche` and loss fund permanently

Summary

After maturity of Tranche, all users are intended to quit and get back their fund and profit. But actually the last user might fail to quit and permanently loss fund due to rounding error in the Tranche.withdraw(). With the consideration there would be many Tranche instances based on different adapters and different maturity, many users would suffer this risk, and this vulnerability would be easily triggered to cause users fund loss.

Vulnerability Detail

The issue arises on L338 of withdraw() function and L676 \~ L680 of _computePrincipalTokenRedeemed(), principalAmount is rounded down in favor of users rather than the protocol, which makes no enough principal token burned . This would cause the protocol have no enough target token to burn for last user.

File: src\Tranche.sol
328:     function withdraw(
329:         uint256 underlyingAmount,
330:         address to,
331:         address from
332:     ) external override nonReentrant expired returns (uint256) {

337:         uint256 sharesRedeem = underlyingAmount.divWadDown(cscale);
338:         uint256 principalAmount = _computePrincipalTokenRedeemed(_gscales, sharesRedeem);
...
343:         _burnFrom(from, principalAmount);
...
350:     }

File: src\Tranche.sol
668:     function _computePrincipalTokenRedeemed(
669:         GlobalScales memory _gscales,
670:         uint256 _shares
671:     ) internal view returns (uint256) {
...
674:         if ((_gscales.mscale * MAX_BPS) / _gscales.maxscale >= oneSubTilt) {
...
676:             return (_shares.mulWadDown(_gscales.mscale) * MAX_BPS) / oneSubTilt;
677:         }
...
680:         return _shares.mulWadDown(_gscales.maxscale);
681:     }

The coded PoC shows more specific procedures how this would occur:


// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {TestTranche} from "./Tranche.t.sol";
import "forge-std/console2.sol";

contract TrancheRoundIssue is TestTranche {
    address alice = address(0x011);
    address bob = address(0x22);
    function setUp() public virtual override {
        super.setUp();
    }

    function testTrancheRoundIssue() public {
        // 1. issue some PT and YT
        deal(address(underlying), address(this), 1_000e6, true);
        tranche.issue(address(this), 1_000e6);

        // 2. alice and bob hold some YT
        yt.transfer(alice, yt.balanceOf(address(this)) / 2);
        yt.transfer(bob, yt.balanceOf(address(this)));

        // 3. after maturity
        vm.warp(_maturity);
        _simulateScaleIncrease();

        // 4. the withdraw() is called some times, typically causing "1" round error each time
        for (uint256 i; i < 10; ++i) {
            tranche.withdraw(2, address(this), address(this));
        }

        // 5. other operations such as collects, redeems and fee collection
        tranche.redeem(tranche.balanceOf(address(this)), address(this), address(this));
        vm.prank(management);
        tranche.claimIssuanceFees();
        vm.prank(alice);
        tranche.collect();

        // 6. bob becames the last unlucky guy, quit would fail
        vm.startPrank(bob);
        vm.expectRevert("ERC20: transfer amount exceeds balance");
        tranche.collect();
        vm.stopPrank();
    }
}

And the logs:

2024-01-napier\napier-v1> forge test --match-test testTrancheRoundIssue
[⠔] Compiling...
[⠔] Compiling 40 files with 0.8.19
[⠊] Solc 0.8.19 finished in 74.33sCompiler run successful!
[⠒] Solc 0.8.19 finished in 74.33s

Running 1 test for test/unit/TrancheRoundIssue.t.sol:TrancheRoundIssue
[PASS] testTrancheRoundIssue() (gas: 1266016)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.58ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

The last user would fail to quit and permanently loss fund

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/Tranche.sol#L668

Tool used

Manual Review

Recommendation

diff --git a/napier-v1/src/Tranche.sol b/napier-v1/src/Tranche.sol
index 62d9562..457b6c5 100644
--- a/napier-v1/src/Tranche.sol
+++ b/napier-v1/src/Tranche.sol
@@ -673,11 +673,11 @@ contract Tranche is BaseToken, ReentrancyGuard, Pausable, ITranche {
         // If it's a sunny day, PT holders lose `tilt` % of the principal amount.
         if ((_gscales.mscale * MAX_BPS) / _gscales.maxscale >= oneSubTilt) {
             // Formula: principalAmount = (shares * mscale * MAX_BPS) / oneSubTilt
-            return (_shares.mulWadDown(_gscales.mscale) * MAX_BPS) / oneSubTilt;
+            return (_shares.mulWadUp(_gscales.mscale) * MAX_BPS) / oneSubTilt;
         }
         // If it's not a sunny day,
         // Formula: principalAmount = shares * maxscale
-        return _shares.mulWadDown(_gscales.maxscale);
+        return _shares.mulWadUp(_gscales.maxscale);
     }

sherlock-admin3 commented 7 months ago

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

ydspa commented 7 months ago

Escalate This issue should be high, though only Tranche's last user would be affected, but with the consideration there would be many Tranche instances existing concurrently based on different adapters and different maturity, and new Tranches would also be continuously created after maturity of old ones. Therefore, in the whole protocol level, many users would suffer this risk, and this vulnerability would be easily triggered. It doesn't meet Shrelock's rule for Medium(a loss is highly constrained), but is suitable to treate as High (loss of funds without extensive limitations)

sherlock-admin2 commented 7 months ago

Escalate This issue should be high, though only Tranche's last user would be affected, but with the consideration there would be many Tranche instances existing concurrently based on different adapters and different maturity, and new Tranches would also be continuously created after maturity of old ones. Therefore, in the whole protocol level, many users would suffer this risk, and this vulnerability would be easily triggered. It doesn't meet Shrelock's rule for Medium(a loss is highly constrained), but is suitable to treate as High (loss of funds without extensive limitations)

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.

xiaoming9090 commented 7 months ago

Escalate

When the Tranche.withdraw function is rounded down, this means that for each withdrawal, there will be 1 wei PT less that is not being burned. If the withdrawal is executed 10000 times, there will be 10000 wei PT less that is not being burned. So, if someone attempts to claim the last 10000 wei PT, it might revert because there might be insufficient target tokens left. However, if the loss is limited to only 10000 wei PT, it is insignificant as 1 PT = 1 underlying asset on maturity.

I checked the POC, and it shows that the last user (Bob) attempted to call the collect function, which is a function to claim yield. It does not appear to be related to the reported issue where the last user cannot exit. Could you please further elaborate on this?

sherlock-admin2 commented 7 months ago

Escalate

When the Tranche.withdraw function is rounded down, this means that for each withdrawal, there will be 1 wei PT less that is not being burned. If the withdrawal is executed 10000 times, there will be 10000 wei PT less that is not being burned. So, if someone attempts to claim the last 10000 wei PT, it might revert because there might be insufficient target tokens left. However, if the loss is limited to only 10000 wei PT, it is insignificant as 1 PT = 1 underlying asset on maturity.

I checked the POC, and it shows that the last user (Bob) attempted to call the collect function, which is a function to claim yield. It does not appear to be related to the reported issue where the last user cannot exit. Could you please further elaborate on this?

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.

ydspa commented 7 months ago

Escalate

When the Tranche.withdraw function is rounded down, this means that for each withdrawal, there will be 1 wei PT less that is not being burned. If the withdrawal is executed 10000 times, there will be 10000 wei PT less that is not being burned. So, if someone attempts to claim the last 10000 wei PT, it might revert because there might be insufficient target tokens left. However, if the loss is limited to only 10000 wei PT, it is insignificant as 1 PT = 1 underlying asset on maturity.

I checked the POC, and it shows that the last user (Bob) attempted to call the collect function, which is a function to claim yield. It does not appear to be related to the reported issue where the last user cannot exit. Could you please further elaborate on this?

Napier is a typical interest market, there would be two types of investors: PT holders (interest seller) and YT holders (interest buyer), if the last quit user is a YT holder, he/she can only call collect() to quit, then it would be stuck, as collect() must quit with all or nothing. This is the core point of this finding.

massun-onibakuchi commented 7 months ago

Napier Tranche is a typical interest market, there would be two types of investors: PT holders (interest seller) and YT holders (interest buyer), if the last quit user is a YT holder, he/she can only call collect() to quit, then it would be stuck, as collect() must quit with all or nothing. This is the core point of this finding.

Basically YT holder would call collect() and can exit with redeemWithYt which requires the same amount of PT.

it would be stuck, as collect() must quit with all or nothing. all or nothing or "loss fund permanently" can be avoided as follow:

e.g.) Alice is the last user (100 YT) in her address A she transfers some wei of YT to her another address B. Then she calls collect() function from address A. she can get yield with insignificant loss.

Also, typically accrued yield is so small because it is not principal user deposited. e.g. 1 YT-cDAI (maturity 1 year and 1% annual rate) would accrue 0.01 DAI. The actual amount stuck could be smaller because the YT holder can collect the yield even before maturity.

nevillehuang commented 7 months ago

Agree with sponsor @massun-onibakuchi, similar issue applies for #56, albeit the nature of rounding differs, but affected loss amounts is minute and core functionality of interested collection will not be permanently DoSed since there is a workaround.

ydspa commented 7 months ago

Napier Tranche is a typical interest market, there would be two types of investors: PT holders (interest seller) and YT holders (interest buyer), if the last quit user is a YT holder, he/she can only call collect() to quit, then it would be stuck, as collect() must quit with all or nothing. This is the core point of this finding.

Basically YT holder would call collect() and can exit with redeemWithYt which requires the same amount of PT.

it would be stuck, as collect() must quit with all or nothing. all or nothing or "loss fund permanently" can be avoided as follow:

e.g.) Alice is the last user (100 YT) in her address A she transfers some wei of YT to her another address B. Then she calls collect() function from address A. she can get yield with insignificant loss.

Also, typically accrued yield is so small because it is not principal user deposited. e.g. 1 YT-cDAI (maturity 1 year and 1% annual rate) would accrue 0.01 DAI. The actual amount stuck could be smaller because the YT holder can collect the yield before maturity.

I have different idea, from the view of my self as a interest investor: let's say current USD APY is 3%, and current 1-year YT-USD price is $0.01, and i expect that the APY would increase to 6%, then i would buy lots of YT-USD (such as 10M which is worth $100K at beginning) from market and hold YT only to earn 500% potential profit at maturity. But if i hold both PT and YT, i only get 5% profit. I think interest speculation is the fancy part of Napier, if I only want to hold PT and YT together, why i don't hold the underlying asset such as cTokens directly? With no additional fee charged by Napier and suffering no vulnerability risk at Napier end.

All in all, what i want to say is: 1) we can't make sure how users use the protocol, we can expect that both YT only holders (affected), PT only holders (not affected) and PT+YT holders (might affected while YT > PT) would be existing. 2) although the ratio of YT/PT might be low, but the absolute YT tokens affected might still be high, as YT is the speculation trade target, users might hold large YT tokens only.

To sum up: Your point is that if the bug WAS known, then users have solution to decrease losses, but my point is that the issue could be easily triggered but can't be easily found, and once it's triggered before discovering, the LOSS is unconstrained , might be low, might be very large .

P.S. the current cDAI APY is 32.05%, not 1%, the YT's value would be much larger than expected reference: https://v2-app.compound.finance/

massun-onibakuchi commented 7 months ago

This APY chart is more fair. https://defillama.com/yields/pool/cc110152-36c2-4e10-9c12-c5b4eb662143

massun-onibakuchi commented 7 months ago

Also, Some of users would deposit PT into Curve finance Tricrypto pool or Napier Pool (Napier pool is a metapool of the Tricrypto pool). Napier pool creates "dead shares", inspired by well-known Uniswap v2. Virtually that means there is a fraction of PT that can never be withdrawn. Given this background, the likelihood that users will actually be unable to withdraw is quite small.

Clearly the PoC assumes issued PT and YT are never deposited into a pool. It can be said that the PoC makes an assumption that greatly differ from how Napier and general Fixed-rate DeFi would be used. PT is a kind of zero-coupon bond, must be traded on a market and users buy or sell based on the market price and their prediction. Users can't speculate on interest if a market liquidity doesn't exist.

Here I want to say that some PTs are definitely permatnently locked in a pool, the last user would be actually a dead address that holds the dead shares.

massun-onibakuchi commented 7 months ago

To sum up: The finding overlooks how Napier and fixed interest rates market works. Considering the following two points, I think that the vulnerability is considered low.

Even under the strong assumption that no tokens are deposited into the Napier pool, which ignores how Napier works, it is possible to avoid a DoS attack. The actual amount stuck could be small. (1) (2)
In reality, due to the dead shares that are permanently locked in a pool, the last user ends up being the holder of these PT (address(1)).

ydspa commented 7 months ago

Also, Some of users would deposit PT into Curve finance Tricrypto pool or Napier Pool (Napier pool is a metapool of the Tricrypto pool). Napier pool creates "dead shares", inspired by well-known Uniswap v2. Virtually that means there is a fraction of PT that can never be withdrawn. Given this background, the likelihood that users will actually be unable to withdraw is quite small.

Clearly the PoC assumes issued PT and YT are never deposited into a pool. It can be said that the PoC makes an assumption that greatly differ from how Napier and general Fixed-rate DeFi would be used. PT is a kind of zero-coupon bond, must be traded on a market and users buy or sell based on the market price and their prediction. Users can't speculate on interest if a market liquidity doesn't exist.

Here I want to say that some PTs are definitely permatnently locked in a pool, the last user would be actually a dead address that holds the dead shares.

There is no guarantee that deadshare's value always > Tranche.withdraw()'s cumulative round lost, it's very small too (1000wei for uniswap). However, based your additional information, I agree this issue is not high. But according sherlock's rule: Causes a loss of funds but requires certain external conditions or specific states, it's still a Medium issue.

nevillehuang commented 7 months ago

Severity will depend on how @Czar102 interprets the following rule:

The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

cvetanovv commented 7 months ago

From the discussion, I think this issue should remain a Medium.

Czar102 commented 6 months ago

Since the loss is highly constrained as there exists a trivial method to cap it (see https://github.com/sherlock-audit/2024-01-napier-judging/issues/16#issuecomment-1985628392), I'm planning to consider it a low/informational issue.

Czar102 commented 6 months ago

Result: Low Unique

sherlock-admin3 commented 6 months ago

Escalations have been resolved successfully!

Escalation status:

xiaoming9090: accepted
ydspa: rejected

sherlock-admin4 commented 6 months ago

The Lead Senior Watson signed off on the fix.

sherlock-audit / 2024-01-napier-judging

KingNFT - The last user can't quit ````Tranche```` and loss fund permanently #16

The last user can't quit `Tranche` and loss fund permanently

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

sherlock-audit / 2024-01-napier-judging

KingNFT - The last user can't quit ````Tranche```` and loss fund permanently #16

The last user can't quit Tranche and loss fund permanently

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

The last user can't quit `Tranche` and loss fund permanently