search

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

3 stars 2 forks source link

Misc low issues #68

Closed sherlock-admin3 closed 5 months ago

sherlock-admin3 commented 5 months ago

Misc low issues

Low/Info issue submitted by jkoppel

Summary

Grab bag of low issues

Vulnerability Detail

  1. PerAddressContinuousVestingInitializable.sol has a method called __ContinuousVesting_init . This method is misnamed. https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/main/contracts/packages/hardhat/contracts/claim/factory/PerAddressContinuousVestingInitializable.sol#L10

  2. For the continuous vesting distributors, the voteFraction is usually 10000 (and is fixed to this value for the initializable versions), but fractionDenominator is 1e18. This makes the amount of vote token each person has very coarse. Consider setting voteFraction to 1e18 as wel.

  3. Consider removing the call to token.safeTransfer(owner(), diff) in adjust(). If somehow the distributor becomes insolvent (or was never fully funded to begin with), you may want to adjust many people's allotments downward, but this call can prevent admins from doing so. Conversely, if it's removed, token can still be swept.

  4. I recommend calling _transferOwnership in AdvancedDistributorInitializable, rather than in the leaf classes. That way subclasses cannot forget to call it.

  5. Several methods are marked nonReentrant, but other contracts are never called.

  6. The extension of ERC20Permit looks troublesome because it exposes several new public methods, although fortunately all of them are stymied by disabling _approve. Upgrading the OpenZeppelin version will remove the dependency on ERC20Permit.

  7. Not sure, but I think the token name of an AdvancedDistributorInitializable will be stuck as uninitialized. It (and similar things) gets set on the implementation contract during construction, but never on the proxy.

  8. The comment "// vested fraction" in PerAddressTrancheVestingInitializable.sol is incorrect; it should read "fraction denominator."

Impact

Code Snippet

Tool used

Manual Review

Recommendation

ArshaanB commented 5 months ago

1) Agreed, should be renamed to __PerAddressContinuousVesting_init and this version needs to be used in both: PerAddressContinuousVestingInitializable.sol and PerAddressContinuousVestingMerkleDistributor.sol 2) We’ll leave it how it is for now since it’s not causing any issues, but will consider changing the _voteFactor for the people vesting.

ArshaanB commented 5 months ago
  1. I don't understand this part of your explanation: "but this call can prevent admins from doing so".

Additionally, how would you sweep the tokens alternatively, are you talking about using sweepToken()?

jkoppel commented 5 months ago

but this call can prevent admins from doing so

Cannot adjust people's total downward if the contract is not fully funded.

I don't understand this part of your explanation: "but this call can prevent admins from doing so". Additionally, how would you sweep the tokens alternatively, are you talking about using sweepToken()?

Yes

ArshaanB commented 5 months ago

Because it would revert, yes?

In this case, we would have to remember to call adjust() and then sweepToken() for the amount when negative, and it does make sense because we were doing something similar when calling adjust() with a positive amount.

Would be a nice modification.

ArshaanB commented 5 months ago

4) Agreed!

5) It doesn't hurt to have them, right? But if there is some trade-off here can definitely look into reducing them.

6) Can you explain how upgrading the OpenZeppelin version would remove the dependency on ERC20Permit? I don't see the functions that ERC20Permit provides being available elsewhere.

ArshaanB commented 5 months ago

7) I need more details for this point, I don't follow. Are you referencing when a new distributor is created using the deployDistributor() in PerAddressContinuousVestingMerkleDistributorFactory.sol (where the distributor is an AdvancedDistributorInitializable)?

8) Agreed! We can just remove the comment in this case, since it's pretty self-explanatory.

jkoppel commented 5 months ago
  1. Just a minor gas issue
  2. In the version of OZ used, ERC20Votes extends Erc20Permit. In later versions, it does not
  3. Yes. I was never able to run the Forge tests, but I do predict that, if you get the tokenName of a distributor deployed with deployDistributor, then the token name will be incorrect.