sherlock-audit / 2024-05-tokensoft-distributor-contracts-update-judging

3 stars 2 forks source link

samuraii77 - Owners might not be able to adjust the total allocated tokens and the tokens claimable by a user #10

Closed sherlock-admin2 closed 4 months ago

sherlock-admin2 commented 4 months ago

samuraii77

medium

Owners might not be able to adjust the total allocated tokens and the tokens claimable by a user

Summary

Owners might not be able to adjust the total allocated tokens and the tokens claimable by a user

Vulnerability Detail

Owners can call AdvancedDistributor#adjust() in order to change the total allocated tokens and the tokens claimable by a particular user:

function adjust(address beneficiary, int256 amount) external onlyOwner {
    DistributionRecord memory distributionRecord = records[beneficiary];
    require(distributionRecord.initialized, 'must initialize before adjusting');

    uint256 diff = uint256(amount > 0 ? amount : -amount);
    require(diff < type(uint120).max, 'adjustment > max uint120');

    if (amount < 0) {
      // decreasing claimable tokens
      require(total >= diff, 'decrease greater than distributor total');
      require(distributionRecord.total >= diff, 'decrease greater than distributionRecord total');
      total -= diff;
      records[beneficiary].total -= uint120(diff);
      token.safeTransfer(owner(), diff);
    } else {
      // increasing claimable tokens
      total += diff;
      records[beneficiary].total += uint120(diff);
    }
    _reconcileVotingPower(beneficiary); // Mint or burn tokens based on the new total
    emit Adjust(beneficiary, amount);
  }

However, there is a scenario where the owners might not be able to adjust those values at all or will require extra steps to make that happen.

If the owners create the distribution with a particular total amount of tokens to be allocated for a user and they decide they want to adjust it, it might fail due to this line:

token.safeTransfer(owner(), diff);

Owners are supposed to transfer the tokens in manually so they might want to adjust them even before they have actually transferred them in. This will cause the function to DoS as there is no balance in the contract to transfer back to the owner. This also breaks the common pattern the protocol is following - they are always requiring the distribution creator to manually transfer or withdraw the tokens while here they are pushing them to him.

The steps that the owner would have to take in order to actually adjust total are the following:

  1. First, change the merkle tree so users don't backrun the deposit and frontrun the adjust in order to claim these tokens unfairly (or deposit, adjust and withdraw in the same transaction)
  2. Deposit the tokens
  3. Adjust
  4. Withdraw the tokens

As you can see, that is a lot of steps to simply adjust total. As a matter of fact, however, there is also a way for the owners to be completely unable to adjust the value. If the amount that they are trying to adjust is larger than the amount of tokens they have at their disposal, they will not be able to adjust that value at all as they don't have enough tokens to deposit.

Impact

Owners might not be able to adjust the total allocated tokens and the tokens claimable by a user

Code Snippet

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

Tool used

Manual Review

Recommendation

Let the owners manually withdraw their tokens.