Varun_05 - Wrong voteFactor is initilized in PerAddressContinuousVestingInitializable.sol

[sherlock-admin3]

Varun_05

high

Wrong voteFactor is initilized in PerAddressContinuousVestingInitializable.sol

Summary

In PerAddressContinuousVestingInitializable.sol voting power of 1x is intended which implies that the fraction denominator and the vote factor should be equal but currently the vote factor is intialised to a wrong value which decreases the voting power to 10**14 times less than 1x(intended) voting power.

Vulnerability Detail

Following is __ContinuousVesting_init function which performs various other required initialization

 function __ContinuousVesting_init(
        IERC20 _token,
        uint256 _total,
        string memory _uri,
        uint160 _maxDelayTime,
        uint160 _salt,
        address _owner
    ) internal onlyInitializing {
        __AdvancedDistributor_init(
            _token,
            _total,
            _uri,
            10000, // 1x voting power
            10 ** 18, // provides the highest resolution possible for continuous vesting
            _maxDelayTime,
            _salt,
            _owner
        );
    }

As can be seen from the comments that the intended voting power is 1x. Now _AdvancedDistributor_init function is as follows

function __AdvancedDistributor_init(
        IERC20 _token,
        uint256 _total,
        string memory _uri,
        uint256 _voteFactor,
        uint256 _fractionDenominator,
        uint160 _maxDelayTime,
        uint160 _salt,
        address _owner
    ) internal onlyInitializing {
        _setSweepRecipient(payable(_owner));
        voteFactor = _voteFactor;
        emit SetVoteFactor(voteFactor);

        __Distributor_init(_token, _total, _uri, _fractionDenominator);
        __FairQueue_init(_maxDelayTime, _salt);
    }

From the above function it is clear that it accepts the 4th parameter as vote factor and 5th parameter as fraction denominator Now key thing to note is that initially in _ConitnousVesting_init function vote factor is set as 10000 and Fraction denominator is set as 10 18. So now vote factor = 10000 fraction denominator = 10 18

Now lets see the following function

/**
     * @notice Set the voting power of undistributed tokens
     * @param _voteFactor The voting power multiplier as a fraction of fractionDenominator
     * @dev The vote factor can be any integer. If voteFactor / fractionDenominator == 1,
     * one unclaimed token provides one vote. If voteFactor / fractionDenominator == 2, one
     * unclaimed token counts as two votes.
     */
    function setVoteFactor(uint256 _voteFactor) external onlyOwner {
        voteFactor = _voteFactor;
        emit SetVoteFactor(voteFactor);
    }

It is clear from the comments mentioned that in order to acheive 1x voting power vote factor should be equal to fraction Denominator which is not followed in the above explained scenario.

Impact

This causes wrong issuance of voting power to the users. Mentioning this as high finding because it causes users less voting power than they should receive thus loss of voting tokens for the users.

Code Snippet

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/factory/PerAddressContinuousVestingInitializable.sol#L22

Tool used

Manual Review

Recommendation

Change the vote factor to 10**18

function __ContinuousVesting_init(
        IERC20 _token,
        uint256 _total,
        string memory _uri,
        uint160 _maxDelayTime,
        uint160 _salt,
        address _owner
    ) internal onlyInitializing {
        __AdvancedDistributor_init(
            _token,
            _total,
            _uri,
@==>           10**18, // 1x voting power
            10 ** 18, // provides the highest resolution possible for continuous vesting
            _maxDelayTime,
            _salt,
            _owner
        );
    }

[vsharma4394]

Escalate

This is a valid issue because it is clearly mentioned in the comments that the intended voting power should be 1x but due to wrong initialization it is set to a wrong value due to which there is loss of voting power for the users. Adding to this ,according to the sherlock rules information in comments stands above all other judging rules thus it is a valid issue.

[sherlock-admin3]

Escalate

This is a valid issue because it is clearly mentioned in the comments that the intended voting power should be 1x but due to wrong initialization it is set to a wrong value due to which there is loss of voting power for the users. Adding to this ,according to the sherlock rules information in comments stands above all other judging rules thus it is a valid issue.

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.

[IronsideSec]

Agreed

According to sherlock's rule version for this contest : Criteria for Issue Validity,

Hierarchy of truth: If the protocol team provides no specific information, the default rules apply (judging guidelines).

If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules. In case of contradictions between the README and CODE COMMENTS, the README is the chosen source of truth.

So the code comment vs code implementation cause this issue and the impact below is also clear. contest readme/docs/code comments as source of truth overrides the general judging rules making this issue valid. From #36

[Hash01011122]

Agreed with the reasoning of @IronsideSec and @vsharma4394, this appears to be a valid issue. What are your thoughts @MxAxM ?

[jkoppel]

I noticed this too, but reported it as a low (see github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update-judging/issues/68 , item 2 ). I'll share my reasoning for reporting it as a low:

This report says the impact is that the issuance of voting power is wrong. My report goes into a bit more detail: that the voting power will become coarse. But coarse votes does not mean anything material will be affected.

With other distributors, if a user has 1 token, then they get 1e18 votes. With this distributor, if a user has 1 token, they get 1e4 votes. To have any material impact, this coarsening would have to flip the outcome of a poll. Like, if users with a combined 10.0001 token vote for option A, and users with a combined 10.00009 token vote for option B, this issue will change the outcome from option A winning to a tie. And that's about the worst that can happen. To me, this is very clearly in the "Low" category.

Another factor in my decision was that I found that the contract had the same issue in the last contest, but it did not make it to the list of issues — and I have trouble imagining no-one noticed it since it's rather glaring.

While it's not in this report and so can't be considered, something that makes this more interesting is if the token has a nonstandard number of decimals, as does USDC. But it's already part of the design that, whenever that's the case, the admin should call setVoteFactor to increase the granularity to be the same as with other tokens! In the case of USDC with only 6 decimals, if they don't, what happens is that everyone gets 0 votes until someone notices and the admin calls setVoteFactor. In the end, no impact.

[Hash01011122]

Had a question here:

if users with a combined 10.0001 token vote for option A, and users with a combined 10.00009 token vote for option B, this issue will change the outcome from option A winning to a tie.

Wouldn't this affect fairness of voting power? Agreed with no impact part for protocol, but it would adversely affect the way protocols voting utility is supposed to function to users. Wouldn't it justify a valid issue @jkoppel? Or it should be neglected as the voting power goes to decimals whose likelihood of happening is low and has no impact.

[jkoppel]

Can I rephrase your question as: "Because the in-scope functionality only deals with distribution, there is no impact on the in-scope functionality. But there's voting stuff which is not in scope, and it could impact the fairness there, by letting a 10.0001 token vote tie with a 10.00009 token vote."

I think the possible change in voting outcomes is still very much a low. It means that, if the admin doesn't call setVoteFactor, if I want to beat your proposal, I'd get to buy 0.00001 token less than I would need to otherwise.

[Hash01011122]

Thanks for clarifying @jkoppel this can be considered as low @MxAxM

[WangSecurity]

@jkoppel as I see, @IronsideSec explains here and in #36 that it results in the voting power for beneficiaries being zero. And I need your additional elaboration regarding the "voting stuff which is not in scope", you mean we don't know how voting will be working (the contracts for voting wasn't present in the codebase), or the contracts are present in the codebase, but out-of-scope, correct?

[jkoppel]

you mean we don't know how voting will be working (the contracts for voting wasn't present in the codebase)

This one.

[IronsideSec]

@WangSecurity

Voting will be used by external protocols to decide governance decisions. There won't be no exetrnal contracts to vote. Whatever balance is there in the distributing contracts, it will used to vote. The fix is not in those external system. It should be on tokensoft. Issue is here and so is the fix.

Agree with LSW that Impact is low , but code is not doing what comment says and the likelihood of issue impact is high. Because we have to state to every client about the bug, that wrong voting factor is set initially. So call setVoteFactor after deployment and then use for governance voting. We don't want external system to see this issue and go they set 10000 / 1e18 to make 1x voting power, so we should change voting power to 1e18 to 1e18. Half of them will miss and will impact their first governance proposal.

[WangSecurity]

Firstly, I agree that this is an issue, but due to the fact that the voteFactor can be easily changed after deployment, I think this issue is indeed low.

Now, from the last comment above, as I understand there's a confusion about these two rules below and I'll explain it:

Hierarchy of truth: If the protocol team provides no specific information, the default rules apply (judging guidelines). If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules. In case of contradictions between the README and CODE COMMENTS, the README is the chosen source of truth. The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality. Issues that break these statements, irrespective of whether the impact is low/unknown, will be assigned Medium severity. High severity will be applied only if the issue falls into the High severity category in the judging guidelines.

Indeed both comments and README are sources of truth above judging rules, but there's a very distinct difference between them.

If the code comments have some information that contradicts the rules, or gives some valuable information to judge issue invalid (e.g. the deposit function has no slippage protection, but comments say it will be added in the future), then yes it would override the rules. But, if there are comments expressing the expected behaviour (like here, the comments clearly show the vote factor has to be 1e18 at least) then it does not mean the issue is automatically medium or high.

Now, let's talk about README. If the README talks about expected/intended functionality/behaviour, but the code is different, then it is valid medium, regardless of impact being low/info.

In other words, if there's discrepancy between code and code comments, but the impact is low, then it is invalid. But if there's discrepancy between the README and the code, but the impact is low, then it is valid medium. Hope this explains the rules.

Hence, as said at the very start, I believe it should be low, since the vote factor can be easily changed before anyone suffers a loss.

Planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• vsharma4394: rejected