Drynooo - User balance is not updated when changing voteFactor

[sherlock-admin2]

Drynooo

high

User balance is not updated when changing voteFactor

Summary

When changing voteFactor, the user's balance was not updated. An attacker can monitor this transaction and vote before the balance is updated, thereby gaining more voting power.

Vulnerability Detail

For example, if voteFactor increases, the voting rights of all users should increase. However, users' voting rights are not updated in time. Attackers can vote immediately after updating their voting rights, thus accounting for a larger proportion. If voteFactor decreases, the voting rights of all users should decrease, and attackers can preempt transactions. Vote before updates to have greater voting power.

Impact

The attacker may have more voting power.

Code Snippet

  function setVoteFactor(uint256 _voteFactor) external onlyOwner {
    voteFactor = _voteFactor;
    emit SetVoteFactor(voteFactor);
  }

Tool used

Manual Review

Recommendation

Update the balances of all users when voteFactor is updated.

[Scorpiondeng]

Escalate

Hi Judge, I think this should be achievable, please re-evaluate.

[sherlock-admin3]

Escalate

Hi Judge, I think this should be achievable, please re-evaluate.

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.

[Hash01011122]

Would be appreciated if you point out the reason on why you think this is valid @Scorpiondeng

[zyppyvids]

Duplicate of #69
I also think that it can be considered a valid medium

[Scorpiondeng]

I now try to give as much detail as possible.

"tokensToVotes" means returning the corresponding number of voting rights, followed by minting or burning through the _reconcileVotingPower function.
If users A, B, and C have the same number of tokens, they should also have the same voting power of 100, totaling 300.
If voteFactor is modified, meaning it increases in the protocol, but the user's balance will not be updated. There are three situations that may lead to unfairness:

1. User A can call claim to withdraw some tokens at this time, thereby updating the voting rights, making A's voting rights greater than one-third of the total.
2. It is also possible that the owner made slight adjustments to a user after modifying voteFactor. Then those users who did not need to adjust did not update their balances based on the latest voteFactor, and their voting rights were also usurped.
3. For those users who have unlocked a lot of tokens but want to have more voting rights but are unwilling to claim, their voting rights balance cannot be updated to get the voting rights they deserve.

Similar to the above assumption, if voteFactor decreases, even if the owner updates each user's voting rights through the adjust function, the attacker can also vote through front-running transactions, voting before reducing his voting power, thus making his voting power greater than one-third.

[jkoppel]

I think this misunderstands an important part of the contract: initializeClaim can be called multiple times, by design. Whenever it's called, the voting rights get updated to the most recent number.

[Scorpiondeng]

You are right, after a while I forgot about this.

But the key point is that when voteFactor increases, not all users will update their voting rights through initialize. For users who do not update, their voting rights will be relatively weakened.
On the other hand, when voteFactor is reduced, even if the project team updates everyone's voting rights, attackers can still use front-running transactions to use more voting rights in advance (Front-running transactions to vote instead of updating balances.).

I have expressed my opinion and respect the judge's decision.

[jkoppel]

So something important to realize is that I and many others pointed out issues with the setVoteFactor function in the last contest, including this one. See sherlock-audit/2023-06-tokensoft-judging#55 . The "increase scenario" there has been fixed, but the "decrease issue" is the exact same as this present report. The protocol team addressed this report and passed fix review, so we can conclude that they do not see what's left as an actual issue. And I would agree. It seems the intention is that, after doing a setVoteFactor, the admin will call initializeClaim on anyone already initialized. For increasing the voting share, the users can be trusted to do it themselves; otherwise, they'd only be hurting themselves.

[Hash01011122]

Agreed with @jkoppel this appears to be a design choice by protocol.

[Scorpiondeng]

In @jkoppel's discussion and previous reports, users' preemptive voting was not considered when voteFactor was reduced.

[jkoppel]

What is preemptive voting? Voting for a proposal before it's created?

[Scorpiondeng]

Before the project party calls initializeClaim on the attacker.

[jkoppel]

Doesn't that require opening a new proposal before the admin's finished adjusting people's voting weight?

Also, since when is voting for a proposal that's already open an attack?

[WangSecurity]

@Scorpiondeng as I understand, by "preemptive voting" you meant that the attacker can front-run admin's call to reduce voteFactor with voting. So they use their entire voting power right before the voting factor gets reduced, cause after it, they'll have less voting power, correct?

[Scorpiondeng]

Yeah. This is basically what it means, or it can be to vote before the administrator calls the initializeDistributionRecord function to update the attacker's voting rights. I think this is unfair to users who vote after updating their voting rights.
As for who initiated the proposal, this is not within the scope of the audit.

[zrax-x]

Hi, @jkoppel @WangSecurity .
Issue#37 points out a similar problem, the key point is that "the user's votes will not take effect immediately after the voteFactor changes". Therefore, my recommendation is to modify the way the user's votes is calculated so that it is always bound to voteFactor.

[WangSecurity]

@jkoppel @Scorpiondeng another small clarification, if there's no proposal live, and the admin reduced the voteFactor, then the attack is not possible, simply cause there are no proposals to vote for?

[jkoppel]

@WangSecurity Correct. If voteFactor is changed when there are no open proposals, then nothing bad can happen from this, so long as everyone voting share is correctly adjusted downward before the next proposal opens.

[WangSecurity]

Hm, then I actually think it would strange for an admin to change the vote factor mid-proposal:

1. There is a proposal and half of the users voted, but the admin increased voting factor, now, voting power of the users who haven't voted yet is higher, which is quite unfair to the users who have already voted
2. Vice versa, there is a proposal and half of the users voted, but the admin decreased voting factor, now, the voting power of the users who haven't voted yet is lower, which is quite unfair to them.

I hope these two examples explain why it would be quite logical to change the vote factor when there' no proposal, cause otherwise, seems like an untrusted admin action.

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:

• Scorpiondeng: rejected