Attackers will steal tokens via frequent distributions

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/reward/CuratorRewardsDistributor.sol#L77-L130

Vulnerability details

Impact

Unfair distribution for intended participants if the attacker backruns most of the minute deposits.

Proof of Concept

See CuratorRewardsDistributor.sol#L77-L130

    function distribute(uint256 credId) external {
        if (!credContract.isExist(credId)) revert InvalidCredId();
        uint256 totalBalance = balanceOf[credId];
        if (totalBalance == 0) {
            revert NoBalanceToDistribute();
        }

        address[] memory distributeAddresses = credContract.getCuratorAddresses(credId, 0, 0);
        uint256 totalNum;

        for (uint256 i = 0; i < distributeAddresses.length; i++) {
            totalNum += credContract.getShareNumber(credId, distributeAddresses[i]);
        }

        if (totalNum == 0) {
            revert NoSharesToDistribute();
        }

        uint256[] memory amounts = new uint256[](distributeAddresses.length);
        bytes4[] memory reasons = new bytes4[](distributeAddresses.length);

        uint256 royaltyfee = (totalBalance * withdrawRoyalty) / RATIO_BASE;
        uint256 distributeAmount = totalBalance - royaltyfee;

        // actualDistributeAmount is used to avoid rounding errors
        // amount[0] = 333 333 333 333 333 333
        // amount[1] = 333 333 333 333 333 333
        // amount[2] = 333 333 333 333 333 333
        uint256 actualDistributeAmount = 0;
        for (uint256 i = 0; i < distributeAddresses.length; i++) {
            address user = distributeAddresses[i];

            uint256 userAmounts = credContract.getShareNumber(credId, user);
            uint256 userRewards = (distributeAmount * userAmounts) / totalNum;

            if (userRewards > 0) {
                amounts[i] = userRewards;
                actualDistributeAmount += userRewards;
            }
        }

        balanceOf[credId] -= totalBalance;

        _msgSender().safeTransferETH(royaltyfee + distributeAmount - actualDistributeAmount);

        //slither-disable-next-line arbitrary-send-eth
        phiRewardsContract.depositBatch{ value: actualDistributeAmount }(
            distributeAddresses, amounts, reasons, "deposit from curator rewards distributor"
        );

        emit RewardsDistributed(
            credId, _msgSender(), royaltyfee + distributeAmount - actualDistributeAmount, distributeAmount, totalBalance
        );
    }

This function is used to distribute the credit balance in the case when (totalBalance != 0), now the logic also includes sending back the caller of the function the difference that occurs due to integer division, i.e https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/reward/CuratorRewardsDistributor.sol#L119-L121

        _msgSender().safeTransferETH(royaltyfee + distributeAmount - actualDistributeAmount);

Issue however is that there is no minimum balance requirement which allows this check to be sidestepped no matter how small the accumulated balance is.

Now since deposits are going to be quite minute and accumulate over time, this then allows an attacker to always backrun blocks where (totalBalance != 0), i.e after deposits from the rewarder contract and in the same case where the round down that occurs here would be massive for the intended distributors, ~0 if deposited amount is quite small and then have the rewards sent to them via CuratorRewardsDistributor.sol#L120.

Recommended Mitigation Steps

Consider having a minimum balance being met before accepting distributions, i.e change this check to:

-        if (totalBalance == 0) {
+        if (totalBalance < minBalanceBeforeDistribute) {
            revert NoBalanceToDistribute();
        }

Assessed type

Other