Loss of funds when using `batchClaiming` function

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L73-L78 https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L103-L117

Vulnerability details

Impact

There is loss of funds when using batchClaiming and function when any user sells all his CurveToken amount is 0

Bug

getClaimableFees function returns 0 amount without accounting for the accumulated holderFees when any user's CurveToken amount is 0 and due to this batchClaiming function will result in holderFee loss.

Proof of Concept

Here is the foundry test file

Here is the test with main logic of the exploit:

    function testWrongClaimableFee() public {
        // Owner sets ExternalFeePercent
        vm.startPrank(owner);
        curves.setMaxFeePercent(5e16);
        curves.setExternalFeePercent(1e16, 1e16, 1e16);
        feeSplitter.setCurves(curves);
        vm.stopPrank();

        // Creator buys first CurveToken
        vm.prank(creator);
        curves.buyCurvesToken(creator, 1);

        // User 1 buys CurveToken
        vm.prank(user);
        curves.buyCurvesToken{value: 1 ether}(creator, 1);

        // Some transactions of CurveToken by some user 2
        vm.prank(user_2);
        curves.buyCurvesToken{value: 1 ether}(creator, 10);

        // If user sells his CurveToken, then his claimable holderFees becomes 0
        vm.startPrank(user);
        curves.sellCurvesToken(creator, 1);
        uint claimableHolderFees = feeSplitter.getClaimableFees(creator, user);
        assertEq(claimableHolderFees, 0);
    }

Tools Used

Foundry

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #951

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[c4-judge]

alcueca marked the issue as not a duplicate

[raymondfam]

There's no logic flaw in batchClaiming() or claimFees(). The loss of funds happens only if a user has onBalanceChange() triggered right before claiming the affected rewards.