search

sherlock-audit / 2024-01-telcoin-judging

6 stars 5 forks source link

araj - A council member loss all his funds when a token is burned #116

Closed sherlock-admin closed 7 months ago

sherlock-admin commented 7 months ago

araj

high

A council member loss all his funds when a token is burned

Summary

A council member will completely losses his TELCOIN balances whenever a token is burned because length of balances array shorten after every burn

Vulnerability Detail

When a member NFT is minted it sets balances array according to tokenId of that minted NFT but when a token is burned, it pops out that index from balances array which shortens the balances array leading to completely loss of funds to a council member

  function burn(
        uint256 tokenId,
        address recipient
    ) external onlyRole(GOVERNANCE_COUNCIL_ROLE) {
     /// code......
        uint256 balance = balances[balances.length - 1];
        balances[tokenId] = balance;
   @>     balances.pop();
        _burn(tokenId);
    }

How that works:-

  1. councilMember minted 5 NFT ie[0, 1, 2, 3, 4]
  2. balances are set for all tokenId ie balances.length = 5 ie [208, 108, 58, 25, 0]
  3. tokenId = 1 is burned ie left tokenId = [0, 2, 3, 4]
  4. length of balances array is also 4 ie [0,1,2,3], and balance in that will be [228, 128, 45, 20], thats how an array works 
    uint256[] public balances;
  5. And balances are associated with tokenId then tokenId = 4 will not be able to withdraw as there is no balance for tokenId = 4, completely losing all his funds

Every time a token is burned a member will loss all his funds

// Here is POC

 it("claiming rewards", async () => {
        //minting 5 NFTs to members, before running make those member1/2/3/4
        await councilMember.mint(member0.address);
        await councilMember.mint(member1.address);
        await councilMember.mint(member2.address);
        await councilMember.mint(member3.address);
        await councilMember.mint(member4.address);

        // burning tokenId = 1
        await councilMember.burn(1, holder.address);

        // member with tokenId 4 not able to withdraw 10 tokens
        await expect(councilMember.connect(member4).claim(4, 10)).to.be
          .reverted;
      });

Impact

A council member will completely loss all his funds

Code Snippet

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

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

Tool used

Manual Review

Recommendation

Use mapping to store balance instead of using an array

+    mapping(uint256 tokenId => uint256 balance) public balances;

Duplicate of #199

sherlock-admin2 commented 7 months ago

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

takarez commented:

valid because {This one is a dupplicate of 109 but minimal impact }