honeymewn - Fund recovery mechanism is broken for merkle vault

[sherlock-admin2]

honeymewn

medium

Fund recovery mechanism is broken for merkle vault

Stuck funds for Merkle Vault strategy in case recipient fails to claim them

Vulnerability Detail

    function withdraw(uint256 _amount) external onlyPoolManager(msg.sender) {
        if (block.timestamp <= allocationEndTime + 30 days) {
            revert INVALID();
        }

        IAllo.Pool memory pool = allo.getPool(poolId);

        if (_amount > poolAmount) {
            revert INVALID();
        }

        poolAmount -= _amount;

        // Transfer the tokens to the 'msg.sender' (pool manager calling function)
        _transferAmount(pool.token, msg.sender, _amount);
    }

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

Merkle strategies have awithdraw() function which only supports withdrawing pool.token. However, since these strategies support multiple tokens through allowedList a sponsor can allocate non pool.token tokens to a pool. Assuming this is a merkle vault the funds will be held by the contract. If a partcipant fails to claim them for some reason the pool manager won't be able to withdraw them. distribute won't work either since it only supports pool.token.

Impact

In case of emergency -- participant failed to claim tokens for Merkle Vault strategy it'll be stuck in the contract with no way to recover.

Code Snippet

Tool used

Manual Review Consider reimplementing withdraw:

Recommendation

    function withdraw(address token, uint256 _amount) external onlyPoolManager(msg.sender) {
        if (block.timestamp <= allocationEndTime + 30 days) {
            revert INVALID();
        }

        // Transfer the tokens to the 'msg.sender' (pool manager calling function)
        _transferAmount(token, msg.sender, _amount);
    }

[sherlock-admin]

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

n33k commented:

invalid, tokens other than pool.token sending to pool is user mistake

[justefg]

Escalate.

Please reread the report. There's an allowList for merkle strategies which is set during intitialization. If someone sends a token from allowlist other than pool.token it can get stuck. Refer to DonationVotingMerkleDistributionBase.t.sol for more info.

[sherlock-admin2]

Escalate.

Please reread the report. There's an allowList for merkle strategies which is set during intitialization. If someone sends a token from allowlist other than pool.token it can get stuck. Refer to DonationVotingMerkleDistributionBase.t.sol for more info.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[0xSulpiride]

Escalate

See above https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/893#issuecomment-1763181144

[sherlock-admin2]

Escalate

See above https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/893#issuecomment-1763181144

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0x00ffDa]

In case of emergency -- participant failed to claim tokens for Merkle Vault strategy it'll be stuck in the contract with no way to recover.

I disagree. I don't think the recipients need to do the claiming. DonationVotingMerkleDistributionVaultStrategy.claim() can be called by anyone. So, a pool manager can push the payments out rather than depending on all recipients taking any action.

[thelostone-mc]

Similar to https://github.com/sherlock-audit/2023-09-Gitcoin-judging/issues/719 which has been fixed

[justefg]

@thelostone-mc as far as i can see you can see you acknowledged this is an issue. mark it as a duplicate then, why close it?

[justefg]

@0x00ffDa they don't need to do claiming, you're correct. however, if they don't claim this might mean a few things (not a comprehensive list): a. they lost their private key b. they're no longer alive (sorry for this but this is reality these days) c. they're blacklisted (if a token has this functionality)

In all of the above cases it makes sense to have a fund recovery mechanism. Now, as for the severity the possibility of this is pretty low but the impact can be pretty high if there's a lot of funds allocated to one participant, for example, and they lose their key.

[neeksec]

Suggest to mark as low/info.

If a partcipant fails to claim them for some reason the pool manager won't be able to withdraw them.

@0x00ffDa they don't need to do claiming, you're correct. however, if they don't claim this might mean a few things (not a comprehensive list): a. they lost their private key b. they're no longer alive (sorry for this but this is reality these days) c. they're blacklisted (if a token has this functionality)

In all of the above cases it makes sense to have a fund recovery mechanism.

These cases listed are all claimer's fault.

[Evert0x]

Agree with lead judge, planning to reject escalation and keep issue state as is.

[justefg]

@Evert0x @neeksec put yourself in the shoes of a project sponsor. they won't be able to get the funds back. will you respond "sorry it's claimers fault"?

[Evert0x]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xsulpiride: rejected