Part 2
Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Incorrect `totalFeeRecipientsShares` Calculation in `MarketMakingEngineConfigurationBranch::configureFeeRecipient` May Cause Share Updates for Existing Fee Recipients to Revert

federodes

Summary

In the MarketMakingEngineConfigurationBranch::configureFeeRecipient function, the calculation of totalFeeRecipientsShares is flawed as it adds the new share without first subtracting the old share. This can lead to exceeding the maximum fee share limit, causing unnecessary reverts when updating the shares of an existing fee recipient.

Vulnerability Details

In the code, the total fee recipient shares are computed by adding the new share to the total without accounting for the existing share. This incorrect comparison can trigger the FeeRecipientShareExceedsLimit error, even if the total shares would remain within the allowed limit after the update.

if (share > 0) {
UD60x18 totalFeeRecipientsSharesX18 = ud60x18(marketMakingEngineConfiguration.totalFeeRecipientsShares);
​
if (
totalFeeRecipientsSharesX18.add(ud60x18(share)).gt(
ud60x18(Constants.MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES)
)
// @audit-issue Failure to deduct old shares when updating could trigger the FeeRecipientShareExceedsLimit error.
) {
revert Errors.FeeRecipientShareExceedsLimit();
}
}

PoC

Add the following test case to test/integration/market-making/market-making-engine-configuration-branch/configureFeeRecipient/configureFeeRecipient.t.sol to demonstrate the vulnerability.

In this test, we create two fee recipients, each assigned a 50% share. Then, when attempting to reduce the share of the first fee recipient to 10%, the transaction will revert.

function test_RevertWhen_UpdatingFeeReceipientWithOldShares()
external
givenTheSenderIsTheOwner
whenFeeRecipientIsNotZero
{
UD60x18 maxOfSharesX18 = ud60x18(Constants.MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES);
​
uint256 quantityOfFeeRecipients = 2;
​
UD60x18 quantityTotalOfFeeRecipientsX18 = convertToUd60x18(quantityOfFeeRecipients);
​
UD60x18 sharePerFeeRecipientX18 = maxOfSharesX18.div(quantityTotalOfFeeRecipientsX18);
​
for (uint256 i = 0; i < quantityOfFeeRecipients; i++) {
marketMakingEngine.configureFeeRecipient(address(uint160(i + 1)), sharePerFeeRecipientX18.intoUint256());
}
​
// attempting to update the first fee recipient to receive a 10% share instead of a 50% should revert
vm.expectRevert({ revertData: abi.encodeWithSelector(Errors.FeeRecipientShareExceedsLimit.selector) });
marketMakingEngine.configureFeeRecipient(address(uint160(1)), 0.1e18);
}

Impact

The vulnerability causes the system to potentially revert with the FeeRecipientShareExceedsLimit error when attempting to update a fee recipient's share, even if the new share would not push the total over the maximum limit. This could disrupt operations and prevent legitimate fee recipient updates.

Tools Used

Manual code review.

Recommendations

To fix the issue, subtract the old share from the total fee recipient shares before adding the new share. This will ensure the updated total is correctly calculated and prevent unnecessary reverts.

Corrected code:

+ (, uint256 oldFeeRecipientShares) = marketMakingEngineConfiguration.protocolFeeRecipients.tryGet(feeRecipient);
​
if (
totalFeeRecipientsSharesX18
+ .sub(ud60x18(oldFeeRecipientShares)) // Subtract old shares
.add(ud60x18(share)) // Add new share
.gt(ud60x18(Constants.MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES))
)
​
- (, uint256 oldFeeRecipientShares) = marketMakingEngineConfiguration.protocolFeeRecipients.tryGet(feeRecipient);

This adjustment ensures the fee recipient share update is valid and avoids exceeding the maximum configurable limit.

Updates

Lead Judging Commences

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

MarketMakingEngineConfigurationBranch::configureFeeRecipient can cause DOS when attempting to change share value of an existing address

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.