Jeiwan - Disproportional distribution of deposited funds causes some depositors to lose funds

[github-actions]

Jeiwan

high

Disproportional distribution of deposited funds causes some depositors to lose funds

Summary

In situations when deposited funds are spent partially (e.g. ongoing claims in OngoingBountyV1 or fixed tiered payouts in TieredFixedBountyV1), depositors receive disproportional amounts after refunding: some depositors may receive 0 tokens while other depositors may receive full amounts.

Vulnerability Detail

The protocol allows multiple depositors to deposit funds into one bounty contracts. Depositors may also withdraw funds after their deposits have expired.

The four bounty contract types have different spending schemes: AtomicBountyV1 and TieredPercentageBountyV1 allow bounty winners to claim the entire balance of the contracts; OngoingBountyV1 and TieredFixedBountyV1 allow partial claims: after all bounty winners have claim all their rewards, some funds may still be left in the contracts.

In the current implementation, the funds that are left in bounty contract after all rewards have been claimed, aren't distributed (during refunding) among their depositors proportionally. Consider this example:

1. Alice and Bob deposited 40 ETH each to a TieredFixedBountyV1 bounty contract. The bounty contract had multiple tiers defined with different payout amounts on each tier. The sum of all awards on all tiers was 60 ETH.
2. Bounty winners have claimed all their awards from the contract, and there are 40 ETH left in the contract.
3. After their deposits have expired, Alice and Bob refund their deposits. There can be two outcomes:
   1. If Alice claims before Bob, she will get 40 ETH and Bob will get 0. In this case, Alice paid 10 ETH to the prize pool and Bob paid 50 ETH, while their initial contribution was equal.
   2. If Bob claims before Alice, he will get 40 ETH and Alice will get 0. In this case, Bob paid 10 ETH to the prize pool and Alice paid 50 ETH, while their initial contribution was equal.
4. In both of these scenarios, the distribution of deposited funds is not fair: either Alice or Bob loses their entire deposited amount, while the other depositor contributes only a portion of their deposit.

This logic is defined here: a depositor gets all available funds if there's not enough funds to refund the entire deposited amount. In other words, whoever refunds first may receive their entire deposit back, while the one who refunds last risks getting nothing.

Impact

Some depositors to bounty contracts may lose all their funds, while other depositors who deposited equal amounts may have their entire amounts refunded.

Code Snippet

DepositManagerV1.sol#L174-L179

Tool used

Manual Review

Recommendation

Two options can be seen as mitigations:

1. Consider disallowing anyone to deposit funds into bounty contracts and allowing doing that only to bounty minters. This will guarantee that all remained funds can only be withdrawn by the same actor who deposited them. However, this may limit the usability of the protocol since some scenarios may required multiple depositors.
2. Consider turning the bounty contracts into tokenized vaults by implementing EIP-4626, e.g. using this implementation from OpenZeppelin. EIP-4626 is designed to track individual deposits and distribute funds proportionally to them. Notice that all current EIP-4626 implementations are vulnerable to a share price inflation attack. If you decide to implement this mitigation, consider checking this PR that's intended to fix the vulnerability, as well as following this discussion for more details.

Duplicate of #257

[IAm0x52]

Dupe of #257