xiaoming90 - Unclaimed yield is indirectly being taxed

[sherlock-admin2]

xiaoming90

high

Unclaimed yield is indirectly being taxed

Summary

Unclaimed yield is automatically collected and reinvested during the minting of PT+YT, resulting in the yield being subjected to an issuance fee, whereas if the user had collected the unclaimed yield themselves, they would not have been charged a fee against their yield. The consequence of this is a loss of assets for the affected users.

Vulnerability Detail

When users call the issue function to mint PY/YT, their unclaimed yield will be automatically collected and converted to PY/YT. There is no option for users to opt out of this.

Let's assume that Alice's unclaimed yield is 100 Target Tokens and the issuance fee is 5%. When she executes the issue function, the protocol forcefully reinvests her 100 Target Tokens; the protocol will charge an issuance fee of 5% and take away 5 Target Tokens from Alice. Thus, she effectively only left with 95 Target Tokens worth of PT/YT.

This is unfair to Alice; if she had collected the unclaimed yield via the collect function, she would not have been charged a fee and would have received all her 100 Target Tokens. In this case, the protocol indirectly charges a fee against the unclaimed yield of the user who executed the function.

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

File: Tranche.sol
179:     function issue(
180:         address to,
181:         uint256 underlyingAmount
182:     ) external nonReentrant whenNotPaused notExpired returns (uint256 issued) {
..SNIP..
210:         // Deduct the issuance fee from the amount of target token minted + reinvested yield
211:         // Fee should be rounded up towards the protocol (against the user) so that issued principal is rounded down
212:         // Hackmd: F0
213:         // ptIssued
214:         // = (u/s + y - fee) * S
215:         // = (sharesUsed - fee) * S
216:         // where u = underlyingAmount, s = current scale, y = reinvested yield, S = maxscale
217:         uint256 sharesUsed = sharesMinted + accruedInTarget;
218:         uint256 fee = sharesUsed.mulDivUp(issuanceFeeBps, MAX_BPS);
219:         issued = (sharesUsed - fee).mulWadDown(_maxscale);

Impact

Loss of assets for the affected users.

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider any of the solutions to mitigate the issue:

1) Avoid charging an issuance fee against reinvested yield 2) Avoid reinvesting the user's unclaimed yield when issuing PT/TY 3) Provide the users an option to opt out of reinvesting of their yield

[sherlock-admin]

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

tsvetanovv commented:

I think we can consider this a "desing decision"

takarez commented:

invalid: same as issue 083

[nevillehuang]

Escalate

I believe this issue is a valid medium severity issue as there is a mismatch between the use of collect() and issue() to claim yield and reinvest yield respectively. Normally, this would be considered users not understanding the use case of the protocol, so would be considered invalid on the basis of user mistake. However, due to the root cause presented in #83 of unauthorized issuance, users would and can unnecessarily be charged a fee for target tokens every single time by other users (do not require a front-run) calling issue() instead of the owner of the yield calling collect(). So I believe the sponsors should consider reviewing this issue @massun-onibakuchi. Why is a issuance tax imposed on users reinvesting yield?

Edit: This should be a duplicate of #83

[sherlock-admin2]

Escalate

I believe this issue is a valid medium severity issue as there is a mismatch between the use of collect() and issue() to claim yield and reinvest yield respectively. Normally, this would be considered users not understanding the use case of the protocol, so would be considered invalid on the basis of user mistake. However, due to the root cause presented in #83 of unauthorized issuance, users would and can unnecessarily be charged a fee for target tokens every single time by other users (do not require a front-run) calling issue() instead of the owner of the yield calling collect(). So I believe the sponsors should consider reviewing this issue @massun-onibakuchi. Why is a issuance tax imposed on users reinvesting yield?

Edit: This should be a duplicate of #83

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.

[massun-onibakuchi]

@nevillehuang

Why is a issuance tax imposed on users reinvesting yield?

Reinvesting yield can be expressed as separated two user actions. 1. claim yield 2. deposit the yield again Those actions are equivalent to functions collect and issue in order. Imaging a user take two actions in order, A user would receive all accrued yield (collect: () -> output: (underlying) ) At this point, claimable yield is 0 and then user needs to deposit the claimed underlying token (issue: input: (underlying)-> output: (shares) ) with reinvesting 0 yield.

As we can see, fee is charged on manually reinvesting yield. That's why issue function should charges fee on reinvesting yield.

[nevillehuang]

@xiaoming9090 Do you have any opinions from the above comments?

[xiaoming9090]

After reading the sponsor's clarification, I agree with the sponsor that the issuance fee should be charged to the re-invest yield. Otherwise, a certain portion of the PT/YT minted/issued will not be subjected to any fee, which would potentially lead to another issue.

However, this report's main concern is not whether a fee should be charged against the re-invest yield. The report's main concern is that the target tokens that one has accumulated as yield are being forcefully re-invested and then charged an issuance fee when the issue function is executed, even if one does not have the intention to re-invest their yield. So the users affected by this issue is effectively getting less yield than expected due to the issuance fee being charged against their yield. An example has been provided in the report.

Issue 83 further aggravates the issue as it allows anyone or malicious users to trigger the issue function on behalf of the victim. If the issuance fee is 5%, anyone can trigger the issue function with the victim's account, causing them to lose 5% of their yield immediately.

Note that most YT holders do not have the intention of executing the issue function or minting PT/YT. They simply want to hold on to the Yield Token (YT) and only want to be exposed to the yield component of an underlying yield-bearing asset. However, due to this issue, anyone can cause a loss to the YT holders.

Thus, this issue should be valid.

[nevillehuang]

@xiaoming9090 Based on your description, I believe the root cause of the issue stems from #83, so I have edited my escalation accordingly.

[xiaoming9090]

@xiaoming9090 Based on your description, I believe the root cause of the issue stems from #83, so I have edited my escalation accordingly.

It is incorrect that the root cause of this issue is the same as Issue #83. Issue #83 simply aggravates this issue. This issue would still exist without Issue 83.

The issue here is that the users are getting less yield than expected due to the issuance fee being charged against their yield indirectly, while Issue 83 is caused by a lack of access control where anyone could trigger the issue function on behalf of the victim. Thus, these are two separate issues.

[Czar102]

@xiaoming9090 isn't this mechanic a design choice?

[xiaoming9090]

@xiaoming9090 isn't this mechanic a design choice?

@Czar102 I believe we need to be clear about what constitutes design choice during the contest (or maybe in future contests). It seems like a grey area to me, and sometimes, there is a fine, thin line between design choice and logic error. In my opinion, for something to be considered a design choice, this choice/decision needs to be communicated, and the rationale behind making such a choice and its impacts (can be negative or positive) on the users also need to be shared.

To me, this issue nudges more towards a logic error in its fee collection mechanism in the protocol where users affected by this issue are effectively getting less yield than expected due to the issuance fee being charged against their yield.

[Czar102]

I rather see this as a design choice, and it seems @nevillehuang also thinks about it in a similar manner:

Normally, this would be considered users not understanding the use case of the protocol, so would be considered invalid on the basis of user mistake.

I believe we need to be clear about what constitutes design choice during the contest (or maybe in future contests). It seems like a grey area to me, and sometimes, there is a fine, thin line between design choice and logic error.

@xiaoming9090 If the documentation isn't clear about the desired behavior, the auditor needs to ask themselves two questions: "Could this be a valid design in any scenario? Or does making contracts work like that make no sense?". If the workings make some sense, it is a design choice. If the Watson is suspecting that the mechanic is unintended, they should ask the sponsor, and in case their concerns are confirmed, the context justifying the validity of the finding (sponsor's intent) should be shared on the public channel. Always feel free to include me in this process, if needed.

Given that this issue is not a duplicate of #83, I'm planning to reject the escalation and leave the issue as is.

[xiaoming9090]

@Czar102 Got it. Thanks for your clarification.

[Czar102]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: rejected