zraxx - Malicious users can gain more vote shares after `voteFactor` is changed.

[sherlock-admin2]

zraxx

medium

Malicious users can gain more vote shares after voteFactor is changed.

Summary

Malicious users can gain more votes after voteFactor is changed.

Vulnerability Detail

In the current protocol design, the user's vote count will be updated during initializeDistributionRecord and claim. It is worth noting that there is no access control for these two methods. The owner can change voteFactor, but the user's vote count will not be directly affected (instead will take effect the next time initializeDistributionRecord or claim is called). Therefore, an attacker can use this to increase his or her vote share.

When voteFactor is reduced, the attacker calls the claim function with other beneficiaries’ address as arguments, thereby reducing their vote count. When the voteFactor increases, the user calls the initializeDistributionRecord function with his/her own address. Ultimately in both cases the attacker's vote share will be increased.

Impact

Malicious users could increase their vote shares.

Code Snippet

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/main/contracts/packages/hardhat/contracts/claim/abstract/AdvancedDistributor.sol#L191-L194

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

Tool used

Manual Review

Recommendation

Change the way the user's votes are calculated. Use balanceOf to get the shares, and then convert to votes using tokensToVotes. In this way, the user's vote count will take effect immediately after the voteFactor changes, instead of requiring the user to actively trigger an update (i.e., call function initialize or claim).

For example, use this way to get votes:

  function getVotes(address beneficiary) public view returns (uint256) {
    DistributionRecord memory record = records[beneficiary];
    uint256 currentShare = balanceOf(beneficiary);
    return tokensToVotes(currentShares);
  }

Then the function _reconcileVotingPower will be:

  // Update voting power based on distribution record initialization or claims
  function _reconcileVotingPower(address beneficiary) private {
    // current share
    uint256 currentShare = balanceOf(beneficiary);
    // correct share after initialization, claim, or adjustment
    DistributionRecord memory record = records[beneficiary];
    uint256 newShare = record.claimed >= record.total ? 0 : record.total - record.claimed;

    if (currentShare > newShare) {
      // reduce voting power through ERC20Votes extension
      _burn(beneficiary, currentShare - newShare);
    } else if (currentShare  < newShare) {
      // increase voting power through ERC20Votes extension
      _mint(beneficiary, newShare - currentShare);
    }
  }

[MxAxM]

If admin action can break certain assumptions about the functioning of the code, it's not considered a valid issue

[zrax-x]

I know what you mean, my understanding is that we should trust the admin actions. But here, the admin action will lead to the violation of the protocol in any case. Isn't it a issue?

[WangSecurity]

@zrax-x could you please give me links to claim and initializeDistributionRecord. But, have to remember, that due to your issue being un-escalated, I'm afraid I cannot re-judge it. I'll give it a look and may be will do something, but cannot promise anything. Hope you understand.

[zrax-x]

@WangSecurity Thank you for your attention. The link of function initializeDistributionRecord is here, and claim is here. In function initializeDistributionRecord, it will finally call the _reconcileVotingPower (link) to update the votes:

// Update voting power based on distribution record initialization or claims
function _reconcileVotingPower(address beneficiary) private {
  // current potential voting power
  uint256 currentVotes = balanceOf(beneficiary);
  // correct voting power after initialization, claim, or adjustment
  DistributionRecord memory record = records[beneficiary];
  uint256 newVotes = record.claimed >= record.total ? 0 : tokensToVotes(record.total - record.claimed);

  if (currentVotes > newVotes) {
    // reduce voting power through ERC20Votes extension
    _burn(beneficiary, currentVotes - newVotes);
  } else if (currentVotes < newVotes) {
    // increase voting power through ERC20Votes extension
    _mint(beneficiary, newVotes - currentVotes);
  }
}

Here, the number of votes is obtained through the balanceOf function. Therefore, the contract will update the number of votes through the _mint and _burn methods.

As for function claim, it firstly calculates the newest claimableAmount and records it (which is implememted in funtion _executeClaim link). Then it will update the number of votes using function _reconcileVotingPower.

  function _executeClaim(
    address beneficiary,
    uint256 _totalAmount,
    bytes memory data
  ) internal virtual returns (uint256) {
    uint120 totalAmount = uint120(_totalAmount);

    // effects
    if (records[beneficiary].total != totalAmount) {
      // re-initialize if the total has been updated
      _initializeDistributionRecord(beneficiary, totalAmount);
    }

    uint120 claimableAmount = uint120(getClaimableAmount(beneficiary, data));
    require(claimableAmount > 0, 'Distributor: no more tokens claimable right now');

    records[beneficiary].claimed += claimableAmount;
    claimed += claimableAmount;

    return claimableAmount;
  }

It is worth noting that functions initializeDistributionRecord and claim have no access control (so anyone can call them).