Lines of code
https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L63-L87
Vulnerability details
Impact
The FeeSplitter contract is designed to distribute fees among token holders. It employs an accumulator pattern to distribute rewards over time among users who do not sell or withdraw their tokens as ERC20s. This pattern works by maintaining an accumulator representing the cumulative total of the reward rate over time, and updating it for each user every time their balance changes.
However, the current implementation does not update the accumulator associated with each user during token transfers, deposits, or withdrawals. The onBalanceChange() function, which is responsible for updating the accumulator following changes in a user's balance, is exclusively called from Curves._transferFees(), which is only called during buy and sell transactions.
This oversight can be easily exploited to drain the FeeSplitter contract of its fees. An attacker could repeatedly transfer the same tokens to new accounts and claim fees every time. This is possible because data.userFeeOffset[account] will be zero for every new account they transfer to, while the claimable rewards are calculated using the current balance returned by the Curves contract.
Since there is no limit to the amount of rewards that may accumulate in the FeeSplitter contract, this can be considered loss of matured yield and is hence classified as high severity.
Proof of Concept
Consider the following scenario:
- Alice buys any amount of curve tokens.
- Alice transfers her tokens to a new account, Account1, by calling
transferCurvesToken().
- The
onBalanceChange() function is not triggered during the transfer, so data.userFeeOffset[account] for Account1 is not updated.
- Account1 can now call
FeeSplitter.claimFees() and will receive fees as if it had been holding the tokens since the beginning, as data.userFeeOffset[account] for this account is 0.
https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L63-L88
function updateFeeCredit(address token, address account) internal {
TokenData storage data = tokensData[token];
uint256 balance = balanceOf(token, account);
if (balance > 0) {
uint256 owed = (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance;
data.unclaimedFees[account] += owed / PRECISION;
data.userFeeOffset[account] = data.cumulativeFeePerToken;
}
}
...
function claimFees(address token) external {
updateFeeCredit(token, msg.sender);
uint256 claimable = getClaimableFees(token, msg.sender);
if (claimable == 0) revert NoFeesToClaim();
tokensData[token].unclaimedFees[msg.sender] = 0;
payable(msg.sender).transfer(claimable);
emit FeesClaimed(token, msg.sender, claimable);
}
- Alice creates another new account, Account2, and transfers the tokens from Account1 to Account2.
- The
onBalanceChange(token, account) function is not triggered during this transfer, so data.userFeeOffset[account] for Account2 is zero.
- Account2 can now claim fees.
- Alice can repeat this process, creating new accounts and transferring tokens between them, to drain the contract of its fees.
Tools Used
Manual review
Recommended Mitigation Steps
To mitigate this issue, the onBalanceChange(token, account) function should be triggered during all token transfers, deposits, and withdrawals. For token transfers, it should be triggered for both accounts. This will ensure that userFeeOffset is accurately tracked, preventing users from claiming fees multiple times.
Assessed type
Other
Lines of code
https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L63-L87
Vulnerability details
Impact
The
FeeSplittercontract is designed to distribute fees among token holders. It employs an accumulator pattern to distribute rewards over time among users who do not sell or withdraw their tokens as ERC20s. This pattern works by maintaining an accumulator representing the cumulative total of the reward rate over time, and updating it for each user every time their balance changes.However, the current implementation does not update the accumulator associated with each user during token transfers, deposits, or withdrawals. The
onBalanceChange()function, which is responsible for updating the accumulator following changes in a user's balance, is exclusively called fromCurves._transferFees(), which is only called during buy and sell transactions.This oversight can be easily exploited to drain the
FeeSplittercontract of its fees. An attacker could repeatedly transfer the same tokens to new accounts and claim fees every time. This is possible becausedata.userFeeOffset[account]will be zero for every new account they transfer to, while the claimable rewards are calculated using the current balance returned by the Curves contract.Since there is no limit to the amount of rewards that may accumulate in the
FeeSplittercontract, this can be considered loss of matured yield and is hence classified as high severity.Proof of Concept
Consider the following scenario:
transferCurvesToken().onBalanceChange()function is not triggered during the transfer, sodata.userFeeOffset[account]for Account1 is not updated.FeeSplitter.claimFees()and will receive fees as if it had been holding the tokens since the beginning, asdata.userFeeOffset[account]for this account is 0.https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L63-L88
onBalanceChange(token, account)function is not triggered during this transfer, sodata.userFeeOffset[account]for Account2 is zero.Tools Used
Manual review
Recommended Mitigation Steps
To mitigate this issue, the
onBalanceChange(token, account)function should be triggered during all token transfers, deposits, and withdrawals. For token transfers, it should be triggered for both accounts. This will ensure thatuserFeeOffsetis accurately tracked, preventing users from claiming fees multiple times.Assessed type
Other