KUMA Protocol - Versus contest - 0x52's results

Judge has assessed an item in Issue #18 as 2 risk. The relevant finding follows:

[L-01] changePayees can result in broken share count Description for (uint256 i; i < newPayees.length; i++) { if (newPayees[i] == address(0)) { revert Errors.CANNOT_SET_TO_ADDRESS_ZERO(); } if (newShares[i] == 0) { revert Errors.SHARE_CANNOT_BE_ZERO(); }

    address payee = newPayees[i];
    _payees.add(payee);
    _shares[payee] = newShares[i];
    _totalShares += newShares[i];

    emit PayeeAdded(payee, newShares[i]);
}

When adding the newPayees to the payees set the contract fails to validate that each address in newPayees is unique. The results is that _totalShares will be incorrect if there is a repeat in newPayees.

Example: Assume newPayees = [Bob,Bob] and newShares = [5,5]. Since _payees.add(payee) is a non-reverting add, if the address already exists in the set it won't revert. The end result is that _totalShares == 10 but _shares[Bob] == 5. This mismatch causes incorrect distribution of rewards.

Lines of code

https://github.com/code-423n4/2023-02-kuma/blob/3f3d2269fcb3437a9f00ffdd67b5029487435b95/src/kuma-protocol/MCAGRateFeed.sol#L75-L97

Vulnerability details

Impact

MCAGRateFeed#getRate may return stale data

Proof of Concept

    (, int256 answer,,,) = oracle.latestRoundData();

Classic C4 issue. getRate only uses answer but never checks the freshness of the data, which can lead to stale bond pricing data. Stale pricing data can lead to bonds being bought and sold on KUMASwap that otherwise should not be available. This would harm KIBToken holders as KUMASwap may accept bond with too low of a coupon and reduce rewards.

Tools Used

Manual Review

Recommended Mitigation Steps

Validate that updatedAt has been updated recently enough:

-   (, int256 answer,,,) = oracle.latestRoundData();
+   (, int256 answer,,updatedAt,) = oracle.latestRoundData();


+   if (updatedAt < block.timestamp - MAX_DELAY) {
+       revert();
+   }
    
    if (answer < 0) {
        return _MIN_RATE_COUPON;
    }

Lines of code

https://github.com/code-423n4/2023-02-kuma/blob/3f3d2269fcb3437a9f00ffdd67b5029487435b95/src/kuma-protocol/KUMASwap.sol#L115-L171

Vulnerability details

Impact

Selling bonds with coupons that are already accounted will fail unexpectedly

Proof of Concept

https://github.com/code-423n4/2023-02-kuma/blob/3f3d2269fcb3437a9f00ffdd67b5029487435b95/src/kuma-protocol/KUMASwap.sol#L116-L118

    if (_coupons.length() == _maxCoupons) {
        revert Errors.MAX_COUPONS_REACHED();
    }

The above lines will cause ALL bonds sales to revert when _coupons.length has reached _maxCoupons. Since bonds may share the same coupon, the swap should continue to accept bonds with a coupon that already exist in the _coupons set.

Tools Used

Manual Review

Recommended Mitigation Steps

sellBond should only revert if the max length has been reached and bond.coupon doesn't already exist:

-   if (_coupons.length() == _maxCoupons) {
+   if (_coupons.length() == _maxCoupons && !_coupons.contains(bond.coupon)) {
        revert Errors.MAX_COUPONS_REACHED();
    }

    function test_sellBond_WithExistingCouponWhenMaxCouponsReached() external {
        _KUMASwap.sellBond(1);
        IKUMABondToken.Bond memory bond_ = _bond;

        for (uint256 i; i < 364; i++) {
            bond_.coupon = bond_.coupon + 1;
            _KUMABondToken.issueBond(address(this), bond_);
            _KUMASwap.sellBond(i + 2);
        }

        _KUMABondToken.issueBond(address(this), _bond);

        _KUMASwap.sellBond(365);
    }

QA Report

Low Risk Findings

[L-01] changePayees can result in broken share count

Description

    for (uint256 i; i < newPayees.length; i++) {
        if (newPayees[i] == address(0)) {
            revert Errors.CANNOT_SET_TO_ADDRESS_ZERO();
        }
        if (newShares[i] == 0) {
            revert Errors.SHARE_CANNOT_BE_ZERO();
        }

        address payee = newPayees[i];
        _payees.add(payee);
        _shares[payee] = newShares[i];
        _totalShares += newShares[i];

        emit PayeeAdded(payee, newShares[i]);
    }

When adding the newPayees to the payees set the contract fails to validate that each address in newPayees is unique. The results is that _totalShares will be incorrect if there is a repeat in newPayees.

Example: Assume newPayees = [Bob,Bob] and newShares = [5,5]. Since _payees.add(payee) is a non-reverting add, if the address already exists in the set it won't revert. The end result is that _totalShares == 10 but _shares[Bob] == 5. This mismatch causes incorrect distribution of rewards.

Lines of Code

https://github.com/code-423n4/2023-02-kuma/blob/3f3d2269fcb3437a9f00ffdd67b5029487435b95/src/kuma-protocol/KUMAFeeCollector.sol#L152-L189

Mitigation

Require that _payees.add(payee) actually adds the address:

        address payee = newPayees[i];
-       _payees.add(payee);
+       require(_payees.add(payee));
        _shares[payee] = newShares[i];
        _totalShares += newShares[i];

[L-02] expireBond can be called on bonds that are already part of the _expiredBonds set

Description

    if (!_bondReserve.contains(tokenId)) {
        revert Errors.INVALID_TOKEN_ID();
    }

    IKUMAAddressProvider KUMAAddressProvider = _KUMAAddressProvider;

    if (IKUMABondToken(KUMAAddressProvider.getKUMABondToken()).getBond(tokenId).maturity > block.timestamp) {
        revert Errors.BOND_NOT_MATURED();
    }

    _expiredBonds.add(tokenId);

expireBond only ever checks if the bond is currently in the reserve set but never checks if it is already part of the expired set

Lines of Code

https://github.com/code-423n4/2023-02-kuma/blob/3f3d2269fcb3437a9f00ffdd67b5029487435b95/src/kuma-protocol/KUMASwap.sol#L321-L337

Mitigation

Revert if bond is already part of the expired set:

-   if (!_bondReserve.contains(tokenId)) {
+   if (!_bondReserve.contains(tokenId) || _expiredBonds.contains(tokenId)) {
        revert Errors.INVALID_TOKEN_ID();
    }

[L-03] KUMASwap#setFees doesn't contain any bounds for fees

Description

KUMASwap#setFees allows manager to set fees to whatever they wish. This includes fees values that would break the functionality of the contract.

Lines of Code

https://github.com/code-423n4/2023-02-kuma/blob/3f3d2269fcb3437a9f00ffdd67b5029487435b95/src/kuma-protocol/KUMASwap.sol#L359-L363

Mitigation

Add reasonable bounds for the max fee allowed