sherlock-audit / 2023-09-Gitcoin-judging

11 stars 7 forks source link

Martians - calling distribute before updateDistribution will brick the strategy in DonationVotingMerkleDistributionBaseStrategy #959

Closed sherlock-admin2 closed 11 months ago

sherlock-admin2 commented 11 months ago

Martians

medium

calling distribute before updateDistribution will brick the strategy in DonationVotingMerkleDistributionBaseStrategy

In DonationVotingMerkleDistributionBaseStrategy contract calling distribute function before updateDistribution function will brick the strategy.

Vulnerability Detail

  1. In _distribute function distributionStarted is being set to true if it is false.
      if (!distributionStarted) {
          distributionStarted = true;
     }
  2. But updateDistribution function will revert if distributionStarted is already true.
    if (distributionStarted) {
          revert INVALID();
    } 
  3. But if _distribute is called with empty data before setting merkeRoot, updateDistribution function can no longer be called to set merkeRoot again . This will be brick the funds distribution because merkle proof verification will always fail if merleRoot isn't set.

Impact

fund distribution process will be bricked

Code Snippet

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L615-L617

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L427-L429

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/strategies/donation-voting-merkle-base/DonationVotingMerkleDistributionBaseStrategy.sol#L730-L732

Tool used

Manual Review

Recommendation

@@ -612,6 +600,9 @@ abstract contract DonationVotingMerkleDistributionBaseStrategy is Native, BaseSt
         override
         onlyPoolManager(_sender)
     {
+        if(merkleRoot == bytes32(0)) revert INVALID_MERKLE_ROOT();

        if (!distributionStarted) {
           distributionStarted = true;
        }
sherlock-admin commented 11 months ago

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

n33k commented:

low because it's more like a user error,but a good sugguestion

n33k commented:

invalid, manager error, better to fix