0xlamide - Council Member holding more than 1 nft will result in disbursement of fewer token than expected to Council member during CouncilMember.sol:retrieve

[sherlock-admin]

0xlamide

medium

Council Member holding more than 1 nft will result in disbursement of fewer token than expected to Council member during CouncilMember.sol:retrieve

Summary

This issue deals with council members receiving fewer tokens than expected during distribution of telcoin to existing council members in the retrieve function

Vulnerability Detail

In the retrieve function we can see that individual balance is gotten by dividing final balance by total supply. When one member holds more than one nft in his address, this will affect the result of final balance resulting in lesser telcoin being disbursed to existing members before new members are added

Impact

Existing council member will receive fewer telcoin than expected

Code Snippet

Let's say we have 5 members and we want to disburse 20 worth of tokens amongst them via retrieve function https://github.com/thegamepro21/tcoin/blob/c20dde4c141cf6e8fce356b729b0bd1cdf52d38f/CouncilMember.sol#L287-L300

Individual balance = 20 / 5 = 4 I.e 4 telcoin per member Let's say we have 5 council members, meanwhile one member holds 2 council membership Nfts in his wallet Individual balance = 20 / 6= 3 due to rounding

3 telcoin is distributed to members instead of 4. The difference between individual balance of both scenerio will be significantly high if final balance is high, we use 20 here fore simplicity

Tool used

Manual Review

Recommendation

Ensure nft balance of address is equal to 0 before minting nft to users or replacing them with existing users https://github.com/thegamepro21/tcoin/blob/c20dde4c141cf6e8fce356b729b0bd1cdf52d38f/CouncilMember.sol#L181-L182

https://github.com/thegamepro21/tcoin/blob/c20dde4c141cf6e8fce356b729b0bd1cdf52d38f/CouncilMember.sol#L134-L135

Check should be done before direct transfer of nft from one address to another

Duplicate of #199

[sherlock-admin2]

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

takarez commented:

invalid because { I consider this issue as invalid even though i dont think thats a supposed behavior(minting 2 for same wallet);lets say its the intended behaviour ; the mint function has an only governor modifier that allows only governace to mint and accroding to sherlock rules its a trusted party; they will ensure that no user get access to 2 per wallet even if thats the intended behaviour even though i dont believe it is.}