KUMA Protocol - Versus contest - bin2chen's results

Lines of code

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

Vulnerability details

Impact

using temporary variables to update balances is a dangerous construction. If transferred to yourself, it will cause your balance to increase, thus growing the token balance infinitely

Proof of Concept

KIBToken overrides _transfer() to perform the transfer of the token, the code is as follows:

    function _transfer(address from, address to, uint256 amount) internal override {
        if (from == address(0)) {
            revert Errors.ERC20_TRANSFER_FROM_THE_ZERO_ADDRESS();
        }
        if (to == address(0)) {
            revert Errors.ERC20_TRANSER_TO_THE_ZERO_ADDRESS();
        }
        _refreshCumulativeYield();
        _refreshYield();

        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;//<--------if from==to,this place Will overwrite the reduction above
        }

        emit Transfer(from, to, amount);
    }

From the code above we can see that using temporary variables "newToBaseBalance" to update balances

Using temporary variables is a dangerous construction.

If the from and to are the same, the balance[to] update will overwrite the balance[from] update simplify the example:

Suppose: balance[alice]=10 , and execute transferFrom(from=alice,to=alice,5) Define the temporary variable: temp_variable = balance[alice]=10 so update the steps as follows:

1.balance[to=alice] = temp_variable - 5 =5 2.balance[from=alice] = temp_variable + 5 =15

after alice transferred it to herself, the balance was increased by 5

The test code is as follows:

add to KIBToken.transfer.t.sol

    //test from == to
    function test_transfer_same() public {
        _KIBToken.mint(_alice, 10 ether);
        assertEq(_KIBToken.balanceOf(_alice), 10 ether);
        vm.prank(_alice);
        _KIBToken.transfer(_alice, 5 ether);   //<-----alice transfer to alice 
        assertEq(_KIBToken.balanceOf(_alice), 15 ether); //<-----increases 5 eth
    }

forge test --match test_transfer_same -vvv

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

Tools Used

Recommended Mitigation Steps

a more general method is use:

balance[to]-=amount
balance[from]+=amount

In view of the complexity of the amount calculation, if the code is to be easier to read, it is recommended：

    function _transfer(address from, address to, uint256 amount) internal override {
        if (from == address(0)) {
            revert Errors.ERC20_TRANSFER_FROM_THE_ZERO_ADDRESS();
        }
        if (to == address(0)) {
            revert Errors.ERC20_TRANSER_TO_THE_ZERO_ADDRESS();
        }
        _refreshCumulativeYield();
        _refreshYield();

+       if (from != to) {
	        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;
	        }
+       }
        emit Transfer(from, to, amount);
    }

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

L-01 changePayees() Suggest adding to check whether newPayees are duplicated to avoid _totalShares error

function changePayees(address[] calldata newPayees, uint256[] calldata newShares) external override onlyManager {

...

    for (uint256 i; i < newPayees.length; i++) {

•    if (_payees.contains(newPayees[i])) {
  

•          revert Errors.PAYEE_ALREADY_EXISTS();
  

•    }    
  
        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]);
    }
  

  }

L-01 changePayees() Suggest adding to check whether newPayees are duplicated to avoid _totalShares error

    function changePayees(address[] calldata newPayees, uint256[] calldata newShares) external override onlyManager {
...

        for (uint256 i; i < newPayees.length; i++) {
+	    if (_payees.contains(newPayees[i])) {
+	          revert Errors.PAYEE_ALREADY_EXISTS();
+	    }    

            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]);
        }
   }

L-02 setFees() Suggest adding a size limit variableFee is the percentage, it is recommended to check that it does not exceed 100% i.e. 1e4

contract KUMASwap is IKUMASwap, PausableUpgradeable, UUPSUpgradeable {
...
    function setFees(uint16 variableFee, uint256 fixedFee) external override onlyRole(Roles.KUMA_MANAGER_ROLE) {
+       require(variableFee < 1e4,"variableFee invalid");
        _variableFee = variableFee;
        _fixedFee = fixedFee;
        emit FeeSet(variableFee, fixedFee);
    }

L-03 KUMABondToken.issueBond() Suggest adding a check whether riskCategory is correct riskCategory is used in many places, especially as a key in addressProvider, it is recommended that issueBond check it

contract KUMABondToken is ERC721, Pausable, IKUMABondToken {
...

   function issueBond(address to, Bond calldata bond)
        external
        override
        onlyRole(Roles.MCAG_MINT_ROLE)
        notBlacklisted(to)
        whenNotPaused
    {
+   	require(bond.riskCategory==keccak256(abi.encode(bond.currency, bond.country, bond.term)));
        _tokenIdCounter.increment();
        uint256 tokenId = _tokenIdCounter.current();
        _bonds[tokenId] = bond; 
        _safeMint(to, tokenId);
        emit BondIssued(tokenId, bond);
    }

L-04 setDeprecationStableCoin() suggests adding the restriction _deprecationInitializedAt needs to be equal to 0 Before entering Deprecated, there will be a buffer time to give the user the right and time to choose, such as whether to buyBond() or sellBond() When enter Deprecated, can only buyBondForStableCoin()/redeemKIBT() both methods are using StableCoin So it is important to use which StableCoin, To avoid changing the StableCoin before the buffer time expires into Deprecated It is recommended that the StableCoin addresss should not be modified after the proposed Deprecated.

    function setDeprecationStableCoin(IERC20 newDeprecationStableCoin)
        external
        override
        onlyRole(Roles.KUMA_MANAGER_ROLE)
        whenNotDeprecated
    {
...
+     require(_deprecationInitializedAt == 0,"_deprecationInitializedAt don't zero");
        if (address(newDeprecationStableCoin) == address(0)) {
            revert Errors.CANNOT_SET_TO_ADDRESS_ZERO();
        }
        emit DeprecationStableCoinSet(address(_deprecationStableCoin), address(newDeprecationStableCoin));
        _deprecationStableCoin = newDeprecationStableCoin;  

L-05 setEpochLength() may still cause _previousEpochCumulativeYield to be smaller than the previous one In order to avoid the previousEpochTimestamp being shifted back after resetting epochLength, we added the check if epochLength > _epochLength then refreshed and then resetting epochLength

    function setEpochLength(uint256 epochLength) external override onlyRole(Roles.KUMA_SET_EPOCH_LENGTH_ROLE) {
        if (epochLength == 0) {
            revert Errors.EPOCH_LENGTH_CANNOT_BE_ZERO();
        }
        if (epochLength > MAX_EPOCH_LENGTH) {
            revert Errors.NEW_EPOCH_LENGTH_TOO_HIGH();
        }     
        if (epochLength > _epochLength) {  //<--------refresh
            _refreshCumulativeYield();
            _refreshYield();
        }
        emit EpochLengthSet(_epochLength, epochLength);
        _epochLength = epochLength;
    }

But epochLength < _epochLength is also possible for the previousEpochTimestamp to be shifted back, because the time is calculated by previousEpochTimestamp = (block.timestamp / epochLength) * epochLength

So it is recommended to remove the epochLength > _epochLength limit:

    function setEpochLength(uint256 epochLength) external override onlyRole(Roles.KUMA_SET_EPOCH_LENGTH_ROLE) {
        if (epochLength == 0) {
            revert Errors.EPOCH_LENGTH_CANNOT_BE_ZERO();
        }
        if (epochLength > MAX_EPOCH_LENGTH) {
            revert Errors.NEW_EPOCH_LENGTH_TOO_HIGH();
        }     
-       if (epochLength > _epochLength) {
            _refreshCumulativeYield();
            _refreshYield();
-       }
        emit EpochLengthSet(_epochLength, epochLength);
        _epochLength = epochLength;
    }