[term-fix] Mitigation Error

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-kuma/blob/22fd56b3f0df71714cb71f1ce2585f1c4dd21d64/src/kuma-protocol/KUMASwap.sol#L177-L220

Vulnerability details

Note - The term refactoring has been made for the following reason:

Our main KIBT is intended to be backed by 1-year treasury bill tokens, however, a bond issued on 1 Jan 2023 does not have the same amount of seconds compared to a 1-year treasury bill issued on 1 March 2023, or 1 Jan 2024 etc.

This seems to be a misunderstanding because a 1-year treasury bill is not 1 year in length. It's actually a 52 week fixed length bond (source), so there's no need for a change. Second as explained below using the new system will cause compatibility issues for variable length bonds like treasury notes and other non-US bonds.

Impact

Non-fixed terms will cause variable length bonds with the same nominal yield to have different coupons

Proof of Concept

Currently coupon is used to track which bonds can and cannot be sold to KUMASwap. Coupon is the amount of yield the bond accumulates per second. Since term is now tracked in months rather than seconds the coupon rate will have to be different between nearly every bond issued. It will also hurt the compatibility of bonds because even when the underlying bond rate doesn't change, the coupon will have to be adjusted.

Example:
There are 31536000 seconds in a calendar year and there are 31622400 seconds in a leap year. This means that bonds that include the end of a leap year will have to have a different (and lower) coupon than bonds with the same yield that don't include the end of a leap year. This is because bonds that do will have a longer term and even though their yields are the same nominally (i.e. 3.5%) they're coupons will be different.

3.5% (non-leap year) => coupon = 1.00000000109085
3.5% (leap year) => coupon = 1.00000000108785

The result of this is that bonds that share the same nominal yields will be forced to have different coupons or else their value will be calculated incorrectly. This causes the frustrating problem that two bonds with the same nominal yield will have different coupons if one includes the end of a leap year while the other doesn't. Since bonds that have a coupon lower than the current rate can't be sold to KUMA swap these bonds can't be sold. This becomes increasingly complicated (and more fragmented) the longer the term of the bonds because different issuance dates will includes more or less leap year ends.

It is also worth considering that different types of bonds will behave differently. As an example T-Bills are fixed length bonds. The "1 year" T-Bill is actually a 52 week fixed length bond. The "2 year" T-Notes are variable length bonds which depends on leap years

Tools Used

Manual Review

Recommended Mitigation Steps

Even though some bonds should have slightly longer terms (because of leap years) the previous way a tracking using fixed term length in seconds will provide a better user experience and prevent bonds with the same nominal yield from having different coupons.

[GalloDaSballo]

Interested in the sponsors thoughts

[m19]

@GalloDaSballo this sparked some internal discussions and we are inclined to agree with the warden here. We've actually gone as far as reverting back the change completely.

This helped us realize that our current model only works with fixed-length treasury bills, ie. 13 weeks, 26 weeks, 52 weeks because those are always the same duration. But as pointed out by the warden treasury notes such as the 2year notes are not fixed in length, we found several that were 730 or 731 or even 729 days in length. We will come up with a better solution in a v1.1 or v2 of the protocol to support these.

[GalloDaSballo]

Am considering the coupon value as well as the duration inaccuracies for:

• Risk of value leak / mispricing
• Inconsistency in behavior between different bonds

[m19]

@GalloDaSballo just a quick question: when you're talking about the risk of value leak/mispricing, you're talking about the term-fix branch changes or the original version?

With the initial codebase (so ignoring the changes made in term-fix) we don’t believe there is a leak of value risk, a different term in seconds would just result in different risk category contracts, and limiting it to fixed term treasury bills (many countries issue these, not just US, but also France for example) will solve this.

[GalloDaSballo]

@GalloDaSballo just a quick question: when you're talking about the risk of value leak/mispricing, you're talking about the term-fix branch changes or the original version?

With the initial codebase (so ignoring the changes made in term-fix) we don’t believe there is a leak of value risk, a different term in seconds would just result in different risk category contracts, and limiting it to fixed term treasury bills (many countries issue these, not just US, but also France for example) will solve this.

Thank you for the info, in order to determine Severity I'd have to estimate the risk introduced by the change (on the branch term-fix), we can then have a discussion around reverting the change as a recommendation

Would you say that term-fix introduced a leak of value or would you say it's mostly an inconvenience / inconsistency in behaviour?

[m19]

@GalloDaSballo

I do agree there could've potentially been a leak of value, for example, if we had started issuing 2-year bonds with a term of 24 while the actual bonds could've been 729, 730, or 731 days in duration. And that was our intention as per the PR description after all. This risk would've not been there if we had stuck with the term in seconds since the 729, 730 and 731 bonds would've all been in a different risk category.

The changes to the KUMASwap.sol made it so that bond.term no longer has to be in seconds as it no longer was used for calculations and now could be anything (days, months, years) and thus allow us to group bonds of different durations together.

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GalloDaSballo]

Per the discussion above, the finding has shown the following risk:

• Bonds with the same duration but different coupons
• Which could have caused KumaSwap to behave in an unintended way

The sponsor is choosing to mitigate by changing bond.term to be expressed in an arbitrary unit which will help standardize and group bonds together

The finding has shown a new risk and inconsistent behavior that could have been introduced via the new PR, and for this reason I believe it meets the requirements for Medium Severity

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

Simon-Busch marked the issue as unmitigated