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

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

io10

Summary

There is a logical error in the MarketMakingEngineConfigurationBranch::configureFeeRecipient that causes unwanted reverts when attempting to decrease an existing fee recipient’s share. The function checks the new share against the maximum configurable protocol fee limit before subtracting the old share. As a result, legitimate updates to an existing recipient’s share can incorrectly revert.

Vulnerability Details

The bug occurs in the following lines in MarketMakingEngineConfigurationBranch::configureFeeRecipient :

if (
totalFeeRecipientsSharesX18.add(ud60x18(share)).gt(
ud60x18(Constants.MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES)
)
) {
revert Errors.FeeRecipientShareExceedsLimit();
}
​

This check adds the new share to marketmakingengineconfiguration::totalFeeRecipientsShares before subtracting the old share value. The total should first be decreased by the old share before the new share is added. Otherwise, a legitimate update (from a higher share to a lower share) will revert unnecessarily.

The order of operations in the share calculation is incorrect. The function does not account for the fact that the old share value should first be subtracted from the total fee recipients’ shares before verifying that the new share fits under the maximum limit.

Impact

This bug can block legitimate reconfigurations of fee shares. Administrators may be prevented from decreasing a fee recipient’s share if the sum of current total shares plus the new share appears to exceed the limit—even when the final configuration would be valid. This can halt necessary protocol governance or maintenance procedures, which may include Adjusting fee splits as market conditions change or correcting an over-allocated fee distribution.

Proof Of Code (POC)

​
function test_configurefeerecipientbug() external {
vm.stopPrank();
​
vm.startPrank({ msgSender: users.owner.account });
marketMakingEngine.configureFeeRecipient(users.sasuke.account, 0.6e18);
vm.expectRevert(Errors.FeeRecipientShareExceedsLimit.selector);
marketMakingEngine.configureFeeRecipient(users.sasuke.account, 0.4e18);
}

Tools Used

Manual Review, Foundry

Recommendations

​
function configureFeeRecipient(address feeRecipient, uint256 share) external onlyOwner {
// revert if protocolFeeRecipient is set to zero
if (feeRecipient == address(0)) revert Errors.ZeroInput("feeRecipient");
​
// load market making engine configuration data from storage
MarketMakingEngineConfiguration.Data storage marketMakingEngineConfiguration =
MarketMakingEngineConfiguration.load();
​
(, uint256 oldFeeRecipientShares) = marketMakingEngineConfiguration.protocolFeeRecipients.tryGet(feeRecipient);
​
// update protocol total fee recipients shares value
if (oldFeeRecipientShares > 0) {
UD60x18 totalFeeRecipientsSharesX18 = ud60x18(marketMakingEngineConfiguration.totalFeeRecipientsShares);
UD60x18 previewtotalFeeRecipientsSharesX18 = totalFeeRecipientsSharesX18.sub(ud60x18(oldFeeRecipientShares))
​
if (
previewtotalFeeRecipientsSharesX18.add(ud60x18(share)).gt(
ud60x18(Constants.MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES)
)
) {
revert Errors.FeeRecipientShareExceedsLimit();
}
if (oldFeeRecipientShares > share) {
marketMakingEngineConfiguration.totalFeeRecipientsShares -=
(oldFeeRecipientShares - share).toUint128();
} else {
marketMakingEngineConfiguration.totalFeeRecipientsShares +=
(share - oldFeeRecipientShares).toUint128();
}
} else {
marketMakingEngineConfiguration.totalFeeRecipientsShares += share.toUint128();
}
​
// update protocol fee recipient
marketMakingEngineConfiguration.protocolFeeRecipients.set(feeRecipient, share);
​
// emit event LogConfigureFeeRecipient
emit LogConfigureFeeRecipient(feeRecipient, share);
}
​

The proposed fix properly reorders the logic to subtract the old fee recipient share before adding the new share, avoiding unwanted reverts. By using a “preview” total (subtracting oldFeeRecipientShares from the existing total) and then adding the new share for the limit check, you ensure no valid configuration is blocked when reducing a fee recipient’s share. This solution also maintains the integrity of the maximum fee constraint.

Updates

Lead Judging Commences

inallhonesty Lead Judge 4 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.