sherlock-audit / 2024-05-pooltogether-judging

2 stars 0 forks source link

hash - Using `feePerClaim` for slippage control could result in claimer's making losses during claims #146

Closed sherlock-admin2 closed 1 month ago

sherlock-admin2 commented 1 month ago

hash

medium

Using feePerClaim for slippage control could result in claimer's making losses during claims

Summary

Using feePerClaim for slippage control could result in claimer's making losses during claims

Vulnerability Detail

Claimers can specify a _minFeePerClaim inorder to accommodate for the slippage in fees caused due to other claimer's claiming

link

  function claimPrizes(
    IClaimable _vault,
    uint8 _tier,
    address[] calldata _winners,
    uint32[][] calldata _prizeIndices,
    address _feeRecipient,
    uint256 _minFeePerClaim
  ) external returns (uint256 totalFees) {

    .....

    if (!feeRecipientZeroAddress) {
      feePerClaim = SafeCast.toUint96(_computeFeePerClaim(_tier, _countClaims(_winners, _prizeIndices), prizePool.claimCount()));
=>    if (feePerClaim < _minFeePerClaim) {
        revert VrgdaClaimFeeBelowMin(_minFeePerClaim, feePerClaim);
      }
    }

But this check is not sufficient as the claimers are actually concerned for the price spent in gas vs total rewards earned and to just estimate with the _feePerClaim, they would have to keep it much higher so that a single claim can cover the overhead in gas. Also in case an attempting index was already claimed by another claimer, it will still be considered to increment the already sold count resulting in a decreased feePerClaim while it could've been actually higher

link

  function _computeFeeForNextClaim(
    uint256 _targetFee,
    SD59x18 _decayConstant,
    SD59x18 _perTimeUnit,
    uint256 _elapsed,
    uint256 _sold,
    uint256 _maxFee
  ) internal pure returns (uint256) {
    // @audit higher the _sold, lower the obtained fees. in the current implementation, even if 5 of the total attempted 10 claims have already been claimed, the feePerClaim will be calculated as if 10 are going to be claimed
    uint256 fee = LinearVRGDALib.getVRGDAPrice(
      _targetFee,
      _elapsed,
      _sold,
      _perTimeUnit,
      _decayConstant
    );
    return fee > _maxFee ? _maxFee : fee;

Impact

Claimer's will suffer losses that could've been avoided

Code Snippet

Tool used

Manual Review

Recommendation

Check for the actual count of prize's that will be claimed and revert early based on that

Duplicate of #124