code423n4 commented 1 year ago

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:

User A have a bond token bt1.
User A calls KUMABondToken.setApprovalForAll(B, true), and user B can operate on all user A's bond tokens.
User A is blacklisted.
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);

GalloDaSballo commented 1 year ago

The Warden has shown an inconsistency in implementation for the blacklist functionality

Because a transfer would still be broken, due to transferFrom performing a check on all accounts involved, I agree with Medium Severity

c4-judge commented 1 year ago

GalloDaSballo marked the issue as primary issue

c4-sponsor commented 1 year ago

m19 marked the issue as sponsor confirmed

m19 commented 1 year ago

We confirmed this issue in a test and intend to fix it:

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