Inexistent Slippage Protection

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-kuma/blob/main/src/kuma-protocol/KUMASwap.sol#L166
https://github.com/code-423n4/2023-02-kuma/blob/main/src/kuma-protocol/KUMASwap.sol#L224

Vulnerability details

Impact

All bond evaluations are dynamic within the KUMASwap::sellBond and KUMASwap::buyBond functions, however, they operate with token IDs as input arguments and do not perform any sanitization on the amount of KIB tokens minted or burned respectively.

In turn, this can lead to the user receiving fewer funds than they expected for the sale of a bond or the user paying a higher amount of KIB tokens than they were willing to. As the blockchain state between a transaction's submission to the network and a transaction's execution can differ, this is a significant issue commonly known as a slippage vulnerability.

The issue is especially applicable in the case of a purchase as the evaluation of a bond increases per second based on the implementation of KUMASwap::_getBondValue.

Proof of Concept

N/A

Tools Used

Manual review.

Recommended Mitigation Steps

The codebase should follow the DEX paradigm of an additional parameter signaling the minimum output / maximum input a user is willing to provide for a particular swap. In the case of a sale, the value should be utilized as a minimum amount of KIB tokens they will receive. In the case of a purchase, the value should be utilized as a maximum instead. We should note that regardless of the bond's evaluation, the interest accrued in the KIBToken (getUpdatedCumulativeYield) can also be influenced between a transaction's submission and a transaction's execution in the network by invoking KIBToken::refreshYield.

As an additional point, fees in the KUMASwap contract can be adjusted between a transaction's submission and a transaction's execution rendering the slippage check during the sellBond function applicable as well.

[GalloDaSballo]

The finding looks valid in that a caller may purchase the incorrect amount if the price is changed, this is also a mitigatable MEV vector which can be avoided via a router / slippage check

Thinking Med is appropriate due to leak of value

[c4-sponsor]

m19 marked the issue as sponsor disputed

[m19]

We disagree with the validity: we calculate the value when buying/selling a bond NFT at the last epoch using the metadata of the NFT, so there isn’t a risk of slippage vulnerability.

If a tx takes a long time to confirm, when buying you are expecting to receive more KIBT for the time that has passed since. And same for buying: you are expected to pay extra for the NFT.

We think it's up to the user to set a sensible gas price to avoid paying a different price than expected.

[alex-ppg]

Thanks for the feedback!

The fee that your protocol applies is entirely up to your discretion (no limitations to invocation) and as such the fee can change between a transaction's submission and a transaction's execution. While the scenario may be unlikely, it is valid and can result in unexpected loss-of-value for the end user when invoking sellBond. (re: disagreement with validity)

Gas fees are not the only issue affecting when a transaction gets executed with regard to buyBond; block re-orgs, rapidly rising network congestion etc. are all factors outside the control of the user when it comes to their transaction being processed even with an adequate at-the-time gas fee. I believe that "you are expected to pay extra for the NFT" is an assumption that should be in a controllable manner for the user to ensure they do not end up paying an unfavorably higher amount of funds than they expected to.

Based on the above:

• It is possible to receive less value than expected when selling a bond
• It is possible to pay an amount arbitrarily higher than expected when purchasing a bond

I believe that slippage checks need to be introduced to the protocol.

[m19]

Based on @alex-ppg's comments I definitely would remove the sponsor-disputed label and just disagree with the severity. We still think it's an extremely unlikely scenario where the fee has to be changed between a seller's tx submission onchain and the tx confirmation. But we have to acknowledge that technically these scenarios could happen in the current implementation where a seller receives less than expected and a buyer pays more than expected.

[c4-sponsor]

m19 marked the issue as disagree with severity

[GalloDaSballo]

Generally speaking lack of mintOut is a fairly common issue in AMM like contracts.

In this case, I believe that bondValue can change for 3 reasons:

• Time between placing tx and execution (_getBondValue is based on time passed)
• KIBToken.getUpdatedCumulativeYield which can change the realizedBondValue
• A change in fees that may happen unfortunately

@m19 lmk if I misunderstood and if you think the changes will not impact prices

[GalloDaSballo]

Setup this test to investigate the changes of price over time (due to network fluctuation as well as interest)

function test_sellBond_PokeValue() public {
        _KUMASwap.sellBond(1);

        skip(300 days);
        IKUMABondToken.Bond memory bond_ = _bond;
        bond_.issuance = uint64(block.timestamp);
        _KUMABondToken.issueBond(address(this), bond_);
        _KUMASwap.sellBond(2);

        bond_.issuance = uint64(block.timestamp);
        _KUMABondToken.issueBond(address(this), bond_);
        

        uint256 baseValue = _KUMASwap.getBondBaseValue(2);
        assertEq(baseValue, uint256(10 ether).wadToRay().rayDiv(_YIELD.rayPow(300 days)), "Exp value");

        skip(60 minutes);
        _KUMASwap.sellBond(3);

        assertEq(baseValue, _KUMASwap.getBondBaseValue(3), "60 mins");
    }

I can get the test to fail (value are not the same), when I set the skip to 1 day

function test_sellBond_PokeValue() public {
        _KUMASwap.sellBond(1);

        skip(300 days);
        IKUMABondToken.Bond memory bond_ = _bond;
        bond_.issuance = uint64(block.timestamp);
        _KUMABondToken.issueBond(address(this), bond_);
        _KUMASwap.sellBond(2);

        bond_.issuance = uint64(block.timestamp);
        _KUMABondToken.issueBond(address(this), bond_);
        

        uint256 baseValue = _KUMASwap.getBondBaseValue(2);
        assertEq(baseValue, uint256(10 ether).wadToRay().rayDiv(_YIELD.rayPow(300 days)), "Exp value");

        skip(1 days);
        _KUMASwap.sellBond(3);

        assertEq(baseValue, _KUMASwap.getBondBaseValue(3), "1 day");
    }

The difference is there but marginal

[GalloDaSballo]

The difference seems to be extremely contained.

>>> 9606919193649876111600712464 / 9606919193649876111125169201 * 100
100.0
>>> (9606919193649876111600712464 - 9606919193649876111125169201) / 9606919193649876111125169201 * 100
4.9500079413005956e-18

With the information that I have, considering that the finding has validity, but in the majority of cases will be extremely limited, I believe QA Low to be the most appropriate severity

[GalloDaSballo]

L

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-a

[c4-judge]

GalloDaSballo marked the issue as selected for report

[c4-judge]

GalloDaSballo marked the issue as not selected for report