Doubling of KIBToken balances

[code423n4]

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.

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

Example of a short and sweet description with good POC

[c4-judge]

GalloDaSballo marked the issue as duplicate of #3

[GalloDaSballo]

Removed primary due to inaccuracy in statements
I will not penalize further though as I believe the POC offered covers the exploit sufficiently

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-sponsor]

m19 marked the issue as sponsor confirmed