Ironsidesec - `currentVotes` is not accounted for properly

[sherlock-admin4]

Ironsidesec

high

currentVotes is not accounted for properly

Summary

issue :  wrong currentVotes accounting likelihood: whenever the voting factor is changed, so medium

Vulnerability Detail

Issue flow :

1. A vesting of 100 tokens is initialized at current voting factor of 1.5x making 150 tokens of voting power.
2. Now after a month, the owner changes the voting power to 1x.

After changes, there are two issues depending on the team's intention.

1. when factor chanbged from 1.5 to 1, then the voting tokens should change from 150 to 100. To make this _reconcileVotingPower has to be called, or else voting tokens won't be burnt or minted depending upon the voting factor change.  So it's an issue when someone with 100 voting power is voting with 150 power because the _reconcileVotingPower is not called yet. The team nowhere mentioned that _reconcileVotingPower will be updated for all the beneficiaries and it is unlikely to spend gas costs if the count is > 100. So this is the issue of voting with previous inflated power instead of currently reduced power. But

2. If this is the intended design that new voting factor applies to only new beneficiaries, then there's another issue that anyone can call claim() below for any beneficiary and it will claim all the claimable tokens, and it will burn / mint the voting tokens acccording to current voting power. But the intention is to apply new voting factor to new benefits, hence the claim should have soem acces control, so that only the bennefieciery can update the voting balance. And, as a user, I don't want to claim the vested tokens yet because I am taking part in governance, AcND tech fact anyone can call claim() is an issue here.

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/PerAddressContinuousVestingMerkle.sol#L53-L70

2024-05-tokensoft-distributor-contracts-update\contracts\packages\hardhat\contracts\claim\PerAddressContinuousVestingMerkle.sol

53:   function claim(
... SNIP ...
61:   )
62:     external
63:     validMerkleProof(keccak256(abi.encodePacked(index, beneficiary, totalAmount, start, cliff, end)), merkleProof)
64:     nonReentrant
65:   {
68:     uint256 claimedAmount = _executeClaim(beneficiary, totalAmount, new bytes(0));
70:     _settleClaim(beneficiary, claimedAmount);
73:   }

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/abstract/AdvancedDistributor.sol#L102

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/abstract/AdvancedDistributor.sol#L78-L92

24-05-tokensoft-distributor-contracts-update\contracts\packages\hardhat\contracts\claim\abstract\AdvancedDistributor.sol

110:   function _executeClaim(
111:     address beneficiary,
112:     uint256 totalAmount,
113:     bytes memory data
114:   ) internal virtual override returns (uint256 _claimed) {
115: >>> _claimed = super._executeClaim(beneficiary, totalAmount, data);
116:     _reconcileVotingPower(beneficiary);
117:   }

79:   function _reconcileVotingPower(address beneficiary) private {
81:     uint256 currentVotes = balanceOf(beneficiary);
83:     DistributionRecord memory record = records[beneficiary];
84:     uint256 newVotes = record.claimed >= record.total ? 0 : tokensToVotes(record.total - record.claimed);
85: 
86:     if (currentVotes > newVotes) {
88:       _burn(beneficiary, currentVotes - newVotes);
89:     } else if (currentVotes < newVotes) {
91:       _mint(beneficiary, newVotes - currentVotes);
92:     }
99:   }

Impact

issue :  wrong currentVotes accounting likelihood : whenever the voting factor is changed, so medium

Code Snippet

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/PerAddressContinuousVestingMerkle.sol#L53-L70

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/abstract/AdvancedDistributor.sol#L102

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/abstract/AdvancedDistributor.sol#L78-L92

Tool used

Manual Review

Recommendation

Either add access control to claim() or chnage teh way voting balance is computed in a way that balanceOf(x) = (voting factor * balanceOf(x) )/ factor denom. So that current balance will always return according to current voting factor.

[IronsideSec]

@MxAxM, please share the rejection response.

[MxAxM]

@MxAxM, please share the rejection response.

According to sherlock docs for invalid issues: An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

It applies here, admin sets a new voting power and it breaks some functionalities