jennifer37 - Incorrect rounding direction in Tranche::withdraw()

[sherlock-admin2]

jennifer37

medium

Incorrect rounding direction in Tranche::withdraw()

Summary

When contract redeem user's underlying tokens by pt tokens, calculate the pt tokens which need to be burned down. Contract uses round down, users can use less pt tokens to get his underlying tokens.

Vulnerability Detail

When users want to redeem their underlying tokens by pt tokens, they can call redeem(). Redeem() will calculate pt tokens needed to be burned according to share amount. And contract uses round down here. It means users can use less pt tokens to redeem underlying tokens than expected. That will be dangerous in some special case.

    function withdraw(
        uint256 underlyingAmount,
        address to,
        address from
    ) external override nonReentrant expired returns (uint256) {
        GlobalScales memory _gscales = gscales;
        uint256 cscale = _updateGlobalScalesCache(_gscales);

        // Compute the shares to be redeemed
        uint256 sharesRedeem = underlyingAmount.divWadDown(cscale);
        round down@==> uint256 principalAmount = _computePrincipalTokenRedeemed(_gscales, sharesRedeem);

        // Update the global scales
        gscales = _gscales;
        // Burn PT tokens from `from`
        @==>_burnFrom(from, principalAmount);
        // Withdraw underlying tokens from the adapter and transfer them to `to`
        _target.safeTransfer(address(adapter), sharesRedeem);
        (uint256 underlyingWithdrawn, ) = adapter.prefundedRedeem(to);

        emit Redeem(from, to, underlyingWithdrawn);
        return principalAmount;
    }

Impact

Rounding direction is not favor of protocol.

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/Tranche.sol#L328-L350 https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/Tranche.sol#L649-L663

Tool used

Manual Review

Recommendation

Suggest to keep round direction in favor of protocol.

[sherlock-admin3]

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

[nevillehuang]

Escalate

This is not a duplicate of #96, but rather a duplicate of #16, albeit it lacks a through impact description and PoC/numerical example so should be invalid.

[sherlock-admin2]

Escalate

This is not a duplicate of #96, but rather a duplicate of #16, albeit it lacks a through impact description and PoC/numerical example so should be invalid.

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]

Escalate

This finding is invalid, as according Sherlock rules:

PoC is required for all issues falling into any of the following groups: ... issues related to precision loss ...

And also mentioned by @nevillehuang this finding misses impact analysis, in this case precision loss is only a QA.

[sherlock-admin2]

Escalate

This finding is invalid, as according Sherlock rules:

PoC is required for all issues falling into any of the following groups: ... issues related to precision loss ...

And also mentioned by @nevillehuang this finding misses impact analysis, in this case precision loss is only a QA.

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.

[Czar102]

Agree with the escalations, planning to invalidate this report. I'm not sure which escalation should I accept, given that the first escalation puts the duplication status before validity. Also, it seems the second escalation was the first one to decidedly bring up invalidation.

Also, @ydspa, the rule you mentioned was well-used. I'm generally trying to treat that section as a best practice, so only findings severely violating it are invalidated. This one is a good example.

[cvetanovv]

I agree with the second escalation. Lack of impact and good explanation make the report Low.

[nevillehuang]

Agree with the escalations, planning to invalidate this report. I'm not sure which escalation should I accept, given that the first escalation puts the duplication status before validity. Also, it seems the second escalation was the first one to decidedly bring up invalidation.

Also, @ydspa, the rule you mentioned was well-used. I'm generally trying to treat that section as a best practice, so only findings severely violating it are invalidated. This one is a good example.

For your consideration, I still mentioned it should be invalid

[Czar102]

Result: Invalid Unique

Given that it wasn't a primary outcome for the first escalation and was for the second one, I'll accept the second instead of the first.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ydspa: accepted
• nevillehuang: rejected

[nevillehuang]

@Czar102 I am unsure why my escalation is rejected. Your reasoning makes little sense to me, given I said this issue should be invalid. Is my comment too confusing for you?

[Czar102]

See the docs:

If there were previous escalations for the same issue (let's say it's the second escalation), it won't be accepted unless the new argument proposes new changes or provides stronger reasons for the changes requested earlier.

This situation is the exception mentioned in the above rule.

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.