sherlock-audit / 2024-10-gamma-rewarder-judging

7 stars 3 forks source link

KupiaSec - Missing implementation on invariant: Total distributed rewards must match initial deposit minus protocol fees #209

Open sherlock-admin4 opened 2 weeks ago

sherlock-admin4 commented 2 weeks ago

KupiaSec

High

Missing implementation on invariant: Total distributed rewards must match initial deposit minus protocol fees

Summary

GammaRewarder contract doesn't implement a verification logic on one of the protocol's invariants: Total distributed rewards must match initial deposit minus protocol fees.

Root Cause

GammaRewarder contract stores information about distribution and claim records of user => token => distribution. However, it lacks any verification logic to check if the total claim amount of a distribution exceeds the total amount to distribute.

This check is crucial and must reside in this contract because this is the place where the claiming records are tracked. Despite some off-chain verification possible using the public state variables, there exists the possibility of manipulation between transactions and inreliability of off-chain operations.

https://github.com/sherlock-audit/2024-10-gamma-rewarder/blob/main/GammaRewarder/contracts/GammaRewarder.sol#L38

https://github.com/sherlock-audit/2024-10-gamma-rewarder/blob/main/README.md#q-what-propertiesinvariants-do-you-want-to-hold-even-if-breaking-them-has-a-lowunknown-impact

Internal pre-conditions

Any distribution is created with non-zero amount and ready to be claimed.

External pre-conditions

None.

Impact

  1. Distribution awards can be claimed beyond the amount than expected. This is possible as there could be other distributions using same token and they all reside in the same contract.
  2. Innocent distributions can get failed due to lack of tokens as they're stolen from attacked distributions.

Mitigation

According to the DistributionParameters struct, total expected amount of a distribution can be calculated using startBlockNumber, endBlockNumber and distributionAmountPerEpoch. (Assume protocol keeps the same blocksPerEpoch. Having this value changed would bring another issue.)

Track the total claimed amount of distribution and ensure it doesn't go beyond the expected amount to claim.