Precission loss in NapierRouter.swapUnderlyingForYt

Summary

Due to division before multplying in NapierRouter.swapUnderlyingForYt uDeposit value become lesser than needed to issue ytOutDesired YT.

Vulnerability Detail

 function swapUnderlyingForYt(
        address pool,
        uint256 index,
        uint256 ytOutDesired,
        uint256 underlyingInMax,
        address recipient,
        uint256 deadline
    ) external payable override nonReentrant checkDeadline(deadline) returns (uint256) {
    ...
            uint256 uDepositNoFee = cscale * ytOutDesired / maxscale; 
            uDeposit = uDepositNoFee * MAX_BPS / (MAX_BPS - (series.issuanceFee + 1)); // 0.01 bps buffer
    ...
}

Division / maxscale happens before multiplying to MAX_BPS, thus leading to precission loss even with bps buffer.

PoC

cscale = 10 ** 7
maxscale = 10 ** 10
fee = 100

def ud(ytout):
    return cscale * ytout // maxscale * 10000 // (10000 - (fee + 1))

def issue(udd):
    shares = udd // cscale
    f = shares * fee // 10000
    return (shares - f) * maxscale

print(issue(ud(1234567891234)))

This snippet in python simulates situation in NapierRouter.swapUnderlyingForYt method and prints out 1230000000000.

Impact

This precission loss results to unexpected reverting of swapUnderlyingForYt or user's deposits loss.

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/v1-pool/src/NapierRouter.sol#L353-L354

Tool used

Manual Review

Recommendation

Change order of division and multiplication in the shown snippet.

sherlock-admin commented 5 months ago

2 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

I don't see division before multiplications in your example

takarez commented:

valid: medium(7)

nevillehuang commented 4 months ago

Escalate

While it is true that the description could be easily misunderstood, I believe this is a duplicate of #101. The "division before multiplication" here is referring to uDepositNoFee dividing by maxScale first, which causes the first round down. Subsequently, the calculation of uDeposit will cause a second round down. While the watson did not mention this thoroughly, I believe the PoC and impact proves the issue sufficiently.

This precission loss results to unexpected reverting of swapUnderlyingForYt or user's deposits loss.

sherlock-admin2 commented 4 months ago

Escalate

While it is true that the description could be easily misunderstood, I believe this is a duplicate of #101. The "division before multiplication" here is referring to uDepositNoFee dividing by maxScale first, which causes the first round down. Subsequently, the calculation of uDeposit will cause a second round down. While the watson did not mention this thoroughly, I believe the PoC and impact proves the issue sufficiently.

This precission loss results to unexpected reverting of swapUnderlyingForYt or user's deposits loss.

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 4 months ago

Escalate

While it is true that the description could be easily misunderstood, I believe this is a duplicate of #101. The "division before multiplication" here is referring to uDepositNoFee dividing by maxScale first, which causes the first round down. Subsequently, the calculation of uDeposit will cause a second round down. While the watson did not mention this thoroughly, I believe the PoC and impact proves the issue sufficiently.

This precission loss results to unexpected reverting of swapUnderlyingForYt or user's deposits loss.

While it is true that Watson (vvv) managed to spot the root cause, which is the rounding error when computing the uDepositNoFee and uDeposit, it fails to clearly describe the impact in the report and did not pinpoint how exactly this round error could lead to a revert in the code or user's deposit loss.

Issue 101 (https://github.com/sherlock-audit/2024-01-napier-judging/issues/101) clearly stated the math and full path that could eventually lead to revert in the code. The reason why a revert occurs at the end is that the rounding down of uDepositNoFee and uDeposit leads to a lower number of PT/YT to be minted/issued, which in turn results in the callback being unable to repay the flash loan since there is a shortfall of PT. Since the flash-loan cannot be repaid, the transaction will revert.

However, the above details are not described in this report. Thus, as per Sherlock's judging guide below, this report only identifies the core issue but does not clearly describe its impact. Thus, it should be considered Low.

In addition to this, there is a submission D which identifies the core issue but does not clearly describe the impact or an attack path. Then D is considered low.