The mitigation results in KIBToken not supporting users to transfer token to themselves (not ERC20 compliant)

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-kuma/blob/f5c7649ecc9650d2f634584e2712014e804f3bbc/src/kuma-protocol/KIBToken.sol#L273-L275

Vulnerability details

Impact

Users will not be able to transfer KIBToken to themselves after the mitigation.
This is not ERC20 compliant.

Proof of Concept

Function KIBToken._transfer() will revert when to == from:

function _transfer(address from, address to, uint256 amount) internal override {
    ...
    if (to == from) {
        revert Errors.CANNOT_TRANSFER_TO_SELF();
    }
    ...
}

Tools Used

VS Code

Recommended Mitigation Steps

We should allow users to transfer KIBToken to themselves, but not perform incorrect calculations

diff --git a/src/kuma-protocol/KIBToken.sol b/src/kuma-protocol/KIBToken.sol
index d1977a5..b9cae75 100644
--- a/src/kuma-protocol/KIBToken.sol
+++ b/src/kuma-protocol/KIBToken.sol
@@ -270,9 +270,6 @@ contract KIBToken is IKIBToken, ERC20PermitUpgradeable, UUPSUpgradeable {
         if (to == address(0)) {
             revert Errors.ERC20_TRANSER_TO_THE_ZERO_ADDRESS();
         }
-        if (to == from) {
-            revert Errors.CANNOT_TRANSFER_TO_SELF();
-        }
         _refreshCumulativeYield();
         _refreshYield();

@@ -280,6 +277,11 @@ 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;

[IAm0x52]

I raised the same concern for H-01 => #11

[alex-ppg]

As far as I know, the EIP-20 standard does not state anything about self-transfers and tokens have implemented this protection mechanism in the past. Additionally, C4 itself has advised the check to be implemented in historical contests such as Notional. I do see the point, however, I am not sure I see a "vulnerability" per se.

[IAm0x52]

The ERC-20 standard does not specify anything about self-transfers but not allowing self transfers is an atypical behavior which can cause integration problems as outlined in my submission #11

[henryfour]

The ERC-20 standard does not specify anything about self-transfers but not allowing self transfers is an atypical behavior which can cause integration problems as outlined in my submission #11

I agree. And it's easy to change to support self transfers, there's no need for this special treatment

[GalloDaSballo]

Finding is valid, but I don't think it breaks ERC20, no comment is made in terms of self-transfer

Since they are idempotent, I don't believe this to be a spec-breaking issue

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Downgrading to QA as I don't believe self-transfer to be part of the ERC-20 spec

[GalloDaSballo]

Agree with finding as QA, closing for awarding purposes