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

2 stars 1 forks source link

aman - off by 1 case `diff < type(uint120).max` in adjust function #48

Closed sherlock-admin4 closed 4 weeks ago

sherlock-admin4 commented 1 month ago

aman

medium

off by 1 case diff < type(uint120).max in adjust function

Summary

While during adjustment of distribution record the check for diff value is off by one.

Vulnerability Detail

The owner will call the adjust function to override the distribution record of users. To do so the owner will provide the user address and the amount to override the record by.

2024-05-tokensoft-distributor-contracts-update/contracts/packages/hardhat/contracts/claim/factory/AdvancedDistributorInitializable.sol:124
124:     function adjust(address beneficiary, int256 amount) external onlyOwner {
125:         DistributionRecord memory distributionRecord = records[beneficiary];
126:         require(distributionRecord.initialized, "must initialize before adjusting");
127: 
128:         uint256 diff = uint256(amount > 0 ? amount : -amount);
129:         require(diff < type(uint120).max, "adjustment > max uint120");
130: 
131:         if (amount < 0) {
132:             // decreasing claimable tokens
133:             require(total >= diff, "decrease greater than distributor total");
134:             require(distributionRecord.total >= diff, "decrease greater than distributionRecord total");
135:             total -= diff;
136:             records[beneficiary].total -= uint120(diff);
137:             token.safeTransfer(owner(), diff);
138:         } else {
139:             // increasing claimable tokens
140:             total += diff;
141:             records[beneficiary].total += uint120(diff);
142:         }
143:         _reconcileVotingPower(beneficiary);
144:         emit Adjust(beneficiary, amount);
145:     }

129 it can be observed that diff must be less then type(uint120).max. which is wrong as we have allowed the user to receive type(uint120).max value. if the owner want to reset the user to 0, the owner must provide type(uint120).max value to override user distribution record to 0. lets assume :

  1. Alice is illegible to receive type(uint120).max value .
  2. The owner decided to reset Alice record to 0.
  3. The owner Provided type(uint120).max negative value to override the user record.
  4. The contract will revert because of require(diff < type(uint120).max, "adjustment > max uint120");

Impact

The user Distribution record will not be override if its value is type(uint120).max. although its a big value but could be possible in vesting because the vesting occur on long duration.

Code Snippet

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/main/contracts/packages/hardhat/contracts/claim/factory/AdvancedDistributorInitializable.sol#L124-L145 https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/main/contracts/packages/hardhat/contracts/claim/abstract/AdvancedDistributor.sol#L119-L140

Tool used

Manual Review

Recommendation

change following condition: From :

require(diff < type(uint120).max, "adjustment > max uint120");

To:

require(diff <= type(uint120).max, "adjustment > max uint120");
sherlock-admin2 commented 4 weeks ago

1 comment(s) were left on this issue during the judging contest.

_karanel commented:

low -> owner can call the function twice; no loss of funds