Submitted by santipu_, also found by zhaojie, _eperezok, SpicyMeatball, rvierdiiev, and BowTiedOriole
In the Curves protocol, a holder fee is imposed on all buy and sell transactions. This fee is distributed among share holders via the FeeSplitter contract, incentivizing long-term holding over frequent trading. The _transferFees() function plays a crucial role in this process by transferring the holder fee to FeeSplitter.
This code ensures that onBalanceChange() and addFees() are called only when holdersFeePercent is non-zero, deeming FeeSplitter unnecessary otherwise.
A critical vulnerability arises when the holderFeePercent, initially set to a positive value, is later changed to zero. In this case, previously collected holder fees remain in the FeeSplitter, awaiting distribution. The absence of onBalanceChange() calls in such scenarios allows new shareholders to unjustly claim these fees, leading to a potential theft of funds.
When the holder fee is reduced from a positive value to zero, new shareholders can exploit this to illicitly claim holder fees from FeeSplitter. This theft is not only limited to these new shareholders but can also be perpetuated across multiple addresses, progressively draining the FeeSplitter of its ETH reserves.
The PoC can be executed in a standard Foundry environment for the Curves project. The test is conducted using the command forge test --match-test testSetHolderFeeZero.
The test mimics an attack where a new shareholder exploits the system to claim holder fees that rightfully belong to previous shareholders.
To prevent this exploitation, it is advised to always call onBalanceChange() in _transferFees(), regardless of the holdersFeePercent status. This ensures continuous and accurate tracking of shareholder balances and rightful fee distribution, even when the holder fee is set to zero.
andresaiello (Curves) confirmed, but disagreed with severity and commented:
alcueca (Judge) decreased severity to Medium and commented:
