fibonacci - `CouncilMember`: minting a new token is not possible after burning

[sherlock-admin2]

fibonacci

high

CouncilMember: minting a new token is not possible after burning

Summary

The mint function utilises totalSupply value as tokenId for a new token. The value of the totalSupply decreases when a token is burned. As a result, the next mint uses a tokenId that is already taken.

Vulnerability Detail

The CouncilMember is inherited from the ERC721EnumerableUpgradeable which overrides the _update function and decreases totalSupply value on burning.

    function totalSupply() public view virtual returns (uint256) {
        ERC721EnumerableStorage storage $ = _getERC721EnumerableStorage();
        return $._allTokens.length;
    }
...    
    function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) {
        address previousOwner = super._update(to, tokenId, auth);

        if (previousOwner == address(0)) {
            _addTokenToAllTokensEnumeration(tokenId);
        } else if (previousOwner != to) {
            _removeTokenFromOwnerEnumeration(previousOwner, tokenId);
        }
        if (to == address(0)) {
-->         _removeTokenFromAllTokensEnumeration(tokenId);
        } else if (previousOwner != to) {
            _addTokenToOwnerEnumeration(to, tokenId);
        }

        return previousOwner;
    }
...
    function _removeTokenFromAllTokensEnumeration(uint256 tokenId) private {
        ERC721EnumerableStorage storage $ = _getERC721EnumerableStorage();
        // To prevent a gap in the tokens array, we store the last token in the index of the token to delete, and
        // then delete the last slot (swap and pop).

        uint256 lastTokenIndex = $._allTokens.length - 1;
        uint256 tokenIndex = $._allTokensIndex[tokenId];

        // When the token to delete is the last token, the swap operation is unnecessary. However, since this occurs so
        // rarely (when the last minted token is burnt) that we still do the swap here to avoid the gas cost of adding
        // an 'if' statement (like in _removeTokenFromOwnerEnumeration)
        uint256 lastTokenId = $._allTokens[lastTokenIndex];

        $._allTokens[tokenIndex] = lastTokenId; // Move the last token to the slot of the to-delete token
        $._allTokensIndex[lastTokenId] = tokenIndex; // Update the moved token's index

        // This also deletes the contents at the last position of the array
        delete $._allTokensIndex[tokenId];
-->     $._allTokens.pop();
    }    

POC

diff --git a/telcoin-audit/test/sablier/CouncilMember.test.ts b/telcoin-audit/test/sablier/CouncilMember.test.ts
index 675b89d..9de46d1 100644
--- a/telcoin-audit/test/sablier/CouncilMember.test.ts
+++ b/telcoin-audit/test/sablier/CouncilMember.test.ts
@@ -178,6 +178,13 @@ describe("CouncilMember", () => {
                 });
             });

+            describe("Failure", () => {
+                it("mint reverts after burn", async () => {
+                    await expect(councilMember.burn(0, member.address)).to.not.reverted;
+                    await expect(councilMember.mint(member.address)).to.reverted;
+                });
+            });
+
             describe("Success", () => {
                 it("the correct removal is made", async () => {
                     await expect(councilMember.burn(1, support.address)).emit(councilMember, "Transfer");

Impact

It is impossible to mint a new token after burning.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L181

Tool used

Manual Review

Recommendation

Use separate variable to keep tracking of the next tokenId.

diff --git a/telcoin-audit/contracts/sablier/core/CouncilMember.sol b/telcoin-au
dit/contracts/sablier/core/CouncilMember.sol
index dda827e..2ef1087 100644
--- a/telcoin-audit/contracts/sablier/core/CouncilMember.sol
+++ b/telcoin-audit/contracts/sablier/core/CouncilMember.sol
@@ -45,6 +45,8 @@ contract CouncilMember is
     // Mapping of who can send each NFT index
     mapping(uint256 => address) private _tokenApproval;

+    uint256 nextTokenId;
+
     /* ========== ROLES ========== */
     // Role assigned for the governance council
     bytes32 public constant GOVERNANCE_COUNCIL_ROLE =
@@ -178,7 +184,7 @@ contract CouncilMember is
         }

         balances.push(0);
         }
+        _mint(newMember, nextTokenId++);
     }

     /**

Duplicate of #199

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid because { Also a dupp of (014)}

[0xf1b0]

Escalate

I believe that this is not a duplicate of #199.

The sole reason these issues were deemed duplicates is because they were both fixed in a single PR (which, by the way, we don't have access to and aren't aware of how it was implemented).

Different issues:

• Incorrect balance management
• Incorrect handling of the next token ID
• The token ID doesn't depend on the balances array length

Different impacts:

• Balances cannot be claimed
• Tokens cannot be minted

Different parts of the code and events when the issue is raised:

• Burning
• Minting

Merely because #199 submission mentions both issues and the fix was implemented in a single PR doesn't imply that those two issues are identical.

Such cases are referenced in the Sherlock documentation: "Do not submit multiple issues in a single submission. Even if the 2 completely different issues occur on the same line, please make sure to separately submit them".

[sherlock-admin]

Escalate

I believe that this is not a duplicate of #199.

The sole reason these issues were deemed duplicates is because they were both fixed in a single PR (which, by the way, we don't have access to and aren't aware of how it was implemented).

Different issues:

• Incorrect balance management
• Incorrect handling of the next token ID
• The token ID doesn't depend on the balances array length

Different impacts:

• Balances cannot be claimed
• Tokens cannot be minted

Different parts of the code and events when the issue is raised:

• Burning
• Minting

Merely because #199 submission mentions both issues and the fix was implemented in a single PR doesn't imply that those two issues are identical.

Such cases are referenced in the Sherlock documentation: "Do not submit multiple issues in a single submission. Even if the 2 completely different issues occur on the same line, please make sure to separately submit them".

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[amshirif]

I believe there is a single issue here that propagated multiple negative affects. The inability to do future mint/burn/balance updates are all a result of the fact that the wrong index was removed. This is the reason that they were all fixed in a single PR.

[0xf1b0]

Issue #199 arises from the removal of an item from the balances array at an index that is still in use. This error occurs due to the incorrect use of the totalSupply value from ERC721EnumerableUpgradeable as the index for the next token ID.

Submissions #49 and #61 present these issues individually and provide separate fixes for each. This indicates that each issue exists independently. Even if the balances array were updated correctly, the problem with minting would still persist.

[Alireza-Razavi]

You even say "after burning" yourself:

It is impossible to mint a new token after burning.

burn will brick many things, you just described another impact. The root cause is the same, without calling burn function, the provided impact won't happen This is a duplicate and the root cause is the same.

[nevillehuang]

Agree with sponsor, this issues should remain as duplicates.

[amshirif]

https://github.com/telcoin/telcoin-audit/pull/31

[Evert0x]

Will reject escalation and keep issue state as is, because the root cause is the same.

[Evert0x]

Result: Duplicate of #199 High

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• 0xf1b0: rejected

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/telcoin/telcoin-audit/pull/31.

[sherlock-admin]

The Lead Senior Watson signed off on the fix.