KUMA Protocol - Versus contest - AkshaySrivastav's results

Lines of code

https://github.com/code-423n4/2023-02-kuma/blob/main/src/kuma-protocol/KIBToken.sol#L276-L292

Vulnerability details

Impact

The KIBToken._transfer function overrides the ERC20Upgradeable._transfer function and adds custom logic.

The modified function looks like this:

function _transfer(address from, address to, uint256 amount) internal override {
    // ...

    uint256 startingFromBalance = this.balanceOf(from);
    if (startingFromBalance < amount) {
        revert Errors.ERC20_TRANSFER_AMOUNT_EXCEEDS_BALANCE();
    }
    uint256 newFromBalance = startingFromBalance - amount;
    uint256 newToBalance = this.balanceOf(to) + amount;

    uint256 previousEpochCumulativeYield_ = _previousEpochCumulativeYield;
    uint256 newFromBaseBalance = WadRayMath.wadToRay(newFromBalance).rayDiv(previousEpochCumulativeYield_);
    uint256 newToBaseBalance = WadRayMath.wadToRay(newToBalance).rayDiv(previousEpochCumulativeYield_);

    if (amount > 0) {
        _totalBaseSupply -= (_baseBalances[from] - newFromBaseBalance);
        _totalBaseSupply += (newToBaseBalance - _baseBalances[to]);
        _baseBalances[from] = newFromBaseBalance;
        _baseBalances[to] = newToBaseBalance;
    }

    // ...
}

It can be seen that while performing the transfer of amount tokens, the function cache the token balances of from and to in temporary variables. These cached values do not represent the actual balances of accounts in an edge case.

This implementation of _transfer function can be exploited by any KIBToken holder by passing their own address as the to parameter. When the from and to parameters are equal the function simply doubles the balance of that respective account. So any token holder holding x token at the start of function invocation will have 2x token at the end of invocation.

The bug can be repeated infinitely to gain a huge KIBToken token balance. This huge token balance can be used to drain assets from other contracts of the protocol, as well as to drain liquidity pools of KIBToken.

Proof of Concept

This test case was added to test/kuma-protocol/kib-token/KIBToken.transfer.t.sol and ran using forge test -m test_audit.

function test_audit_doubleMoneyBug() public {
    address alice = makeAddr("alice");
    assertEq(_KIBToken.balanceOf(alice), 0);        // Initial balance is 0

    _KIBToken.mint(alice, 100e18);                  // Tokens minted
    assertEq(_KIBToken.balanceOf(alice), 100e18);   // Balance is now 100e18

    vm.startPrank(alice);
    _KIBToken.transfer(alice, 100e18);              // Tokens transferred to self

    assertEq(_KIBToken.balanceOf(alice), 200e18);   // Balance is doubled 200e18
}

Tools Used

Forge

Recommended Mitigation Steps

Consider adding a check require(from != to) in the _transfer function. Or, always try to reference the storage parameters directly instead of storing values in temporary variables.

Lines of code

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

Vulnerability details

Impact

In KUMAFeeCollector contract, the addPayee validates that an already present payee cannot be added again to the _payees set.

    function addPayee(address payee, uint256 share) external override onlyManager {
        if (_payees.contains(payee)) {
            revert Errors.PAYEE_ALREADY_EXISTS();
        }
        // ...
    }

However a similar check is not present in the changePayees function. https://github.com/code-423n4/2023-02-kuma/blob/main/src/kuma-protocol/KUMAFeeCollector.sol#L152

So a single payee can be added multiple times to the _payees set using the changePayees function.

Proof of Concept

Consider this scenario:

• KUMAFeeCollector is deployed and Account1 and Account2 are intended to be added as payees with 10 share each.
• Using the addPayee function each account can be added only once.
• But using the changePayees function, Account1 can be added twice to the _payees set.

The final payees list looks like this:

• Account1 - 10 shares
• Account2 - 10 shares
• Account1 - 10 shares
• _totalShares = 30

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding a check in changePayees function also which prevent addition of duplicate payee entries.