KUMASwap.buyBond() is vulnerable to being used for reentry attacks

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-kuma/blob/3f3d2269fcb3437a9f00ffdd67b5029487435b95/src/kuma-protocol/KUMASwap.sol#L210-L228 https://github.com/code-423n4/2023-02-kuma/blob/3f3d2269fcb3437a9f00ffdd67b5029487435b95/src/kuma-protocol/KBCToken.sol#L51-L62

Vulnerability details

Impact

KUMASwap.buyBond() could be exploited for some kind of reentry attack now or in the future

Proof of Concept

KUMASwap.buyBond() may trigger a callback to the sender's contract before the following statements being executed:

_updateMinCoupon();
KIBToken.burn(msg.sender, realizedBondValue);

When the user's contract is called back while the contract state update is incomplete, the user can reenter to exploit the incorrect intermediate state for various possible attacks.

The potential reentry attack entry is triggered by: buyBond - > issueBond - > _safeMint() -> msg.sender.onERC721Received()

In this reentry, KUMASwap's _minCoupon state might be incorrect because the bond was bought, but _updateMinCoupon() hasn't been called to updated it yet.

The relevant code of KUMASwap.buyBond() and KBCToken.issueBond() is:

function buyBond(uint256 tokenId) external override whenNotPaused whenNotDeprecated {
    ...
    if (requireClone) {
      _cloneBonds[tokenId] = IKBCToken(KUMAAddressProvider.getKBCToken()).issueBond(
          msg.sender,
          IKBCToken.CloneBond({
              parentId: tokenId,
              issuance: KIBToken.getPreviousEpochTimestamp(),
              coupon: KIBToken.getYield(),
              principal: realizedBondValue
          })
      );
    }
    _updateMinCoupon();
    KIBToken.burn(msg.sender, realizedBondValue);
    ...
}

function issueBond(address to, CloneBond memory cBond) external ... {
    ...
    _safeMint(to, tokenId);
    ...
}

Tools Used

VS Code

Recommended Mitigation Steps

We should move the code that could trigger the callback to the end of the function to make sure that all states have been updated completely:

diff --git a/src/kuma-protocol/KUMASwap.sol b/src/kuma-protocol/KUMASwap.sol
index 7208898..734e506 100644
--- a/src/kuma-protocol/KUMASwap.sol
+++ b/src/kuma-protocol/KUMASwap.sol
@@ -207,6 +207,10 @@ contract KUMASwap is IKUMASwap, PausableUpgradeable, UUPSUpgradeable {

         bool requireClone = bondFaceValue > realizedBondValue;

+        _updateMinCoupon();
+
+        KIBToken.burn(msg.sender, realizedBondValue);
+
         if (requireClone) {
             _cloneBonds[tokenId] = IKBCToken(KUMAAddressProvider.getKBCToken()).issueBond(
                 msg.sender,
@@ -217,13 +221,7 @@ contract KUMASwap is IKUMASwap, PausableUpgradeable, UUPSUpgradeable {
                     principal: realizedBondValue
                 })
             );
-        }
-
-        _updateMinCoupon();
-
-        KIBToken.burn(msg.sender, realizedBondValue);
-
-        if (!requireClone) {
+        } else {
             KUMABondToken.safeTransferFrom(address(this), msg.sender, tokenId);
         }

[c4-judge]

GalloDaSballo marked the issue as duplicate of #15

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

L

[c4-judge]

GalloDaSballo marked the issue as grade-a