alexbabits - Array swap and pop method during burn() leads to complete loss of user rewards and breaks mint()

[sherlock-admin]

alexbabits

high

Array swap and pop method during burn() leads to complete loss of user rewards and breaks mint()

Summary

The swap and pop method during CouncilMembers.burn() leads to loss of user rewards from mismatching of accounting for user balances. This is because there is a fragile connection between the user's NFT tokenId and the tracking of their ERC20 TELCOIN balance rewards via the balances array.

The burn() function also results in a broken CouncilMembers.mint() function because the total supply is reduced, which leads to a state where any future mint will try to create an NFT where the tokenId already exists, since tokenId is based directly on the total supply.

Vulnerability Detail

Background: Inside CouncilMembers.mint(), the balances array length and NFT totalSupply() are increasing at the same rate of 1 per call, via balances.push(0); and _mint(). The tokenId assigned to the new member's NFT is the current totalSupply() before mint.

For example, if totalSupply() is 3 for the NFTs, their tokenId is 3, and their associated ERC20 rewards must always be found at balances[3]. The delicate accounting falls apart during a call to burn().

Exploit Example: Imagine the governor needs to call burn() for tokenId 2 council member's NFT. There are currently 5 holders with different TELCOIN ERC20 reward balances (e18 values).

Even though all rewards are distributed equally among holders every time _retrieve() is called, earlier holders will have accumulated more rewards if _retrieve() calls were made.

    function burn(uint256 tokenId, address recipient) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
        require(totalSupply() > 1, "CouncilMember: must maintain council");
        _retrieve();
        _withdrawAll(recipient, tokenId);
        uint256 balance = balances[balances.length - 1];
        balances[tokenId] = balance;
        balances.pop();
        _burn(tokenId);
    }

balances array state during burn():

[19, 17, 13, 11, 7] = State 0: NFT holders with various TELCOIN balances associated with each of their NFT tokenId's.
[20, 18, 14, 12, 8] = State 1: _retrieve() Juices up all holders balances by 1 for example.
[20, 18, 0, 12, 8] = State 2: _withdrawAll() Sets index 2's balance to 0.
[20, 18, 8, 12, 8] = State 3: balances[tokenId] = balance; Sets index 2's balance to 8.
[20, 18, 8, 12] = State 4: balances.pop(); Removes last index and burns NFT for tokenId 2, reducing the totalSupply() from 5 to 4.

Impact

1. Holder of tokenId 4 can no longer call claim() because it will revert with out of bounds because index 4 doesn't exist in the array. His balance moved from index 4 to index 2, and is now trapped at that location where tokenId 2 was burned.
2. There are now tokenId 0, 1, 3, 4 with a total supply of 4. This means anytime the governor calls mint(), it will attempt to mint an NFT for tokenId 4, but that tokenId already exists, so mint() will always revert and is now broken.

Code Snippet

mint(): https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L180-#L181
burn(): https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L218-#L220
claim(): https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L108-#L110

Tool used

Manual Review

Recommendation

Consider using a mapping instead of an array to track associated rewards balances for NFT holders. Requires heavy modification throughout that is not included below.

-    uint256[] public balances;
+    mapping(uint256 => uint256) // NFT tokenId --> TELCOIN rewards

Duplicate of #199

[sherlock-admin2]

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

takarez commented:

valid because { The watson was able to explain how would that lead to an issues by also burning a token; its also dupp of 109 }