updateRoyalty is not distributing first before update the withdrawRoyalty

[howlbot-integration]

Lines of code

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

Vulnerability details

Basically if the owner want to upgrade the royalty, he just call the updateRoyalty the problem is that the contact need distribute the rewards before changing the updateRoyalty because if is not called the distribute function will distribute a wrong value possibly because the contract does not hold the enough amount of ether in case of the royalty update was increased the value.

  function updateRoyalty(uint256 newRoyalty_) external onlyOwner {
        if (newRoyalty_ > MAX_ROYALTY_RANGE) {
            revert InvalidRoyalty(newRoyalty_);
        }
        withdrawRoyalty = newRoyalty_;
        emit RoyaltyUpdated(newRoyalty_);
    }

[Link]

Impact

Incorrect distribution can be occasioned in case of the owner update the Royalties because there is not distribution before, if the upgrade is up the contract get Dos because there is not more ether in the contract:

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

If the upgrade is down the distribution will be incorrect because is distributing less rewards.

Proof of Concept

Those other similar medium vuln in contest:

setFees doesn't collect previous fees before changing fee values

Lack of update when modifying pool fee

Tools Used

Manual

Recommended Mitigation Steps

Consider get the credIds and distribute this before the update of the withdrawRoyalty

Assessed type

Other

[ZaK3939]

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

1. incorrect sentence; rolaytee is % param
   "the contract does not hold the enough amount of ether in case of the royalty update was increased the value."

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory:
Insufficient proof

[jorgect207]

Hey @ZaK3939 you may don't understand the issue To illustrate properly this imagine the next scenario:

1. totalBalance is 1000
2. withdrawRoyalty is 5%, it that mean that the royaltyfee fee will be 50.
3. Now what happen if this fee percentage is update?, let say that was update to 10%, now the royaltyfee is 100 and not 50 as it should be.

This have being a medium in code4arena that why i add this issues to my favor:

code-423n4/2024-04-noya-findings#544

code-423n4/2024-02-wise-lending-findings#160

[ZaK3939]

@jorgect207
Pls let me know more. i cant understand.

pls check this test, this test succesufully update royalty and sent expected eth
=>[PASS] testUpdateRoyaltyToMax() (gas: 908483)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.68ms (573.92µs CPU time)

 function testUpdateRoyaltyToMax() public {
        uint256 credId = 1;
        uint256 depositAmount = 1 ether;
        uint256 newRoyalty = 1000; // 10% in basis points
        uint256 expectedExecuteRoyalty = depositAmount / 10;

        // Deposit some ETH to the curatorRewardsDistributor
        curatorRewardsDistributor.deposit{ value: depositAmount }(credId, depositAmount);

        // Signal creds for different users
        vm.startPrank(user1);
        vm.deal(user1, bondingCurve.getBuyPriceAfterFee(credId, 1, 1));
        cred.buyShareCred{ value: bondingCurve.getBuyPriceAfterFee(credId, 1, 1) }(credId, 1, 0);
        vm.stopPrank();

        vm.startPrank(user2);
        vm.deal(user2, bondingCurve.getBuyPriceAfterFee(credId, 2, 2));
        cred.buyShareCred{ value: bondingCurve.getBuyPriceAfterFee(credId, 2, 2) }(credId, 2, 0);
        vm.stopPrank();

        // Update royalty to max (1000  = 10%)
        vm.prank(owner);
        curatorRewardsDistributor.updateRoyalty(newRoyalty);

        // Record initial balances
        uint256 initialBalance = anyone.balance;

        // Distribute rewards
        vm.prank(anyone);
        curatorRewardsDistributor.distribute(credId);
        // Check final balances
        uint256 finalBalance = anyone.balance;

        // Assert the distribution
        assertEq(
            finalBalance - initialBalance, expectedExecuteRoyalty, "Executor should receive expectedExecuteRoyalty"
        );

        // Check that the balance in curatorRewardsDistributor is now 0
        assertEq(
            curatorRewardsDistributor.balanceOf(credId),
            0,
            "CuratorRewardsDistributor balance should be 0 after distribution"
        );
    }
    ```

[jorgect207]

hey @ZaK3939 the issue is easy to understands, pleas check the other vulns:

code-423n4/2024-04-noya-findings#544
code-423n4/2024-02-wise-lending-findings#160

Changing fees will recalculate previously accumulated fees if they were not accrued.

Using the test that you provide in the comment i made some modifications to explain the issue, bassicly the amount

 function testUpdateRoyaltyToMax() public {
        uint256 credId = 1;
        uint256 depositAmount = 1 ether;
        uint256 newRoyalty = 1000; // 10% in basis points
        uint256 expectedExecuteRoyalty = depositAmount / 10;


        // Deposit some ETH to the curatorRewardsDistributor
        curatorRewardsDistributor.deposit{ value: depositAmount }(credId, depositAmount);


        // Update royalty to max (1000  = 10%)  //At this point evryone think that they going to get this 10% as rewards
        vm.prank(owner);
        curatorRewardsDistributor.updateRoyalty(newRoyalty); <---------

        // Signal creds for different users
        vm.startPrank(user1);
        vm.deal(user1, bondingCurve.getBuyPriceAfterFee(credId, 1, 1));
        cred.buyShareCred{ value: bondingCurve.getBuyPriceAfterFee(credId, 1, 1) }(credId, 1, 0);
        vm.stopPrank();

        vm.startPrank(user2);
        vm.deal(user2, bondingCurve.getBuyPriceAfterFee(credId, 2, 2));
        cred.buyShareCred{ value: bondingCurve.getBuyPriceAfterFee(credId, 2, 2) }(credId, 2, 0);
        vm.stopPrank();



        // Now the royalty rewards was update to the 7%; but user was expecting a 10% royalty fee, (Note that this could be changed without any malicious intention and the bug still persist)
        newRoyalty = 700; // 7% in basis points  <----------
        vm.prank(owner);
        curatorRewardsDistributor.updateRoyalty(newRoyalty);

        // Record initial balances
        uint256 initialBalance = anyone.balance;

        // Distribute rewards
        vm.prank(anyone);
        curatorRewardsDistributor.distribute(credId);
        // Check final balances
        uint256 finalBalance = anyone.balance;

    }
    ```

See the arrows above; what i mean here is that the fee have to be accrued before that update the fee.

@ZaK3939 letting my discussion to here, thanks so much for your time and patience.

[ZaK3939]

@jorgect207 Thank you. I understand your point, but the original report was highlighting a different issue, which I believe is incorrect.

This is incorrect point out.

contract does not hold the enough amount of ether in case of the royalty update was increased the value.

[jorgect207]

Thank so much for answer @ZaK3939,

Hey so sorry i guess i made an invalid stamen, the correct statement will be if is not called the distribute function will distribute a wrong value letting the contract with less or more royalty's that it should be

I hope this invalid statement does not penalize me to cancel the findings Since the rest is good. 🙏

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)

[c4-judge]

fatherGoose1 marked the issue as grade-a

[fatherGoose1]

Labeling as QA. It may be part of Phi's flow to call distribute() prior to updateRoyalty().