search

sherlock-audit / 2024-04-titles-judging

10 stars 7 forks source link

0x73696d616f - Lost funds due to not validating the number of recipients in `FeeManager`, as the `SplitFactory` and `Wallet` do not validate it #355

Closed sherlock-admin4 closed 5 months ago

sherlock-admin4 commented 5 months ago

0x73696d616f

medium

Lost funds due to not validating the number of recipients in FeeManager, as the SplitFactory and Wallet do not validate it

Summary

SplitV2 and SplitFactory do not validate the number of recipients, which could lead to creators receiving fees in their Split wallets but never being able to withdraw them due to OOG.

Vulnerability Detail

SplitV2 and SplitFactory may only revert if the number of recipients is so large that it runs out of gas when verifying if the sum of the individual allocation of the recipients matches the total allocation, SplitFactoryV2::createSplit() -> SplitWalletV2::initialize() -> SplitV2::validate().

Thus, as distributing assets likely consumes significantly more gas than a simple sum operation, it would likely revert when distributing given enough recipients.

Impact

Lost funds, stuck in the Split wallets due to running OOG when distributing.

Code Snippet

The functionality to limit recipients is missing, so no code snippet. Check the links above for the confirmation that the Split ecossystem does not validate the number of recipients.

Tool used

Manual Review

Vscode

Recommendation

Set an hardcap on the number of recipients (should be different per blockchain). The Split docs mention 2000 maximum recipients.

0x73696d616f commented 4 months ago

Escalate

This should be a valid medium issue, the issue shows how it is possible to add to many recipients and make the transaction revert. The underlying integration warns about this in their docs but there is not cap on it.

sherlock-admin3 commented 4 months ago

Escalate

This should be a valid medium issue, the issue shows how it is possible to add to many recipients and make the transaction revert. The underlying integration warns about this in their docs but there is not cap on it.

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.

cvetanovv commented 4 months ago

This is invalid.

If it happens OOG here, the function will be called with a smaller array. I don't see the problem.

Planning to reject the escalation and leave the issue as is.

0x73696d616f commented 4 months ago

This is indeed invalid but for this reason.

Evert0x commented 4 months ago

Result: Invalid Unique

sherlock-admin4 commented 4 months ago

Escalations have been resolved successfully!

Escalation status: