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

Incorrect Share Limit Validation in `MarketMakingEngineConfigurationBranch.sol#configureFeeRecipient()`

rampage

Summary

A logical error in the share limit validation of configureFeeRecipient() causes valid fee recipient configurations to be incorrectly rejected. The validation occurs before accounting for existing shares, leading to false maximum limit violations even when the final total would be within bounds.

Links to affected code

  • MarketMakingEngineConfigurationBranch.sol:L613~654

Vulnerability details

Impact

The configureFeeRecipient() function in MarketMakingEngineConfigurationBranch.sol performs share limit validation before updating the total shares, which can lead to incorrect rejections of valid configurations. This can prevent legitimate fee recipient share adjustments even when the resulting total would be within allowed limits.

Description

The current implementation validates the maximum share limit by adding the new share amount to the current total, without considering the existing share of the fee recipient being updated. This creates a logical error in the following sequence:

  1. The function checks if currentTotal + newShare > MAX_LIMIT

  2. Only after this check, it adjusts the total by accounting for the old shares

  3. This order of operations means the validation doesn't accurately reflect the final state

For example, if:

  • Current total shares = 800

  • MAX_LIMIT = 1000

  • Existing recipient share = 300

  • New desired share = 400

The transaction would revert because 800 + 400 > 1000, even though the actual final total would be 900 (800 - 300 + 400), which is valid.

Recommended Mitigation

Move the share limit validation after the total shares calculation to ensure it accurately reflects the final state. The validation should occur after adjusting for any existing shares of the fee recipient.

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();
​
- // check if share is greater than zero to verify the total will not exceed the maximum shares
- if (share > 0) {
- UD60x18 totalFeeRecipientsSharesX18 = ud60x18(marketMakingEngineConfiguration.totalFeeRecipientsShares);
-
- if (
- totalFeeRecipientsSharesX18.add(ud60x18(share)).gt(
- ud60x18(Constants.MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES)
- )
- ) {
- revert Errors.FeeRecipientShareExceedsLimit();
- }
- }
​
(, uint256 oldFeeRecipientShares) = marketMakingEngineConfiguration.protocolFeeRecipients.tryGet(feeRecipient);
​
// update protocol total fee recipients shares value
if (oldFeeRecipientShares > 0) {
if (oldFeeRecipientShares > share) {
marketMakingEngineConfiguration.totalFeeRecipientsShares -=
(oldFeeRecipientShares - share).toUint128();
} else {
marketMakingEngineConfiguration.totalFeeRecipientsShares +=
(share - oldFeeRecipientShares).toUint128();
}
} else {
marketMakingEngineConfiguration.totalFeeRecipientsShares += share.toUint128();
}
​
+ // check if share is greater than zero to verify the total will not exceed the maximum shares
+ if (share > 0) {
+ UD60x18 totalFeeRecipientsSharesX18 = ud60x18(marketMakingEngineConfiguration.totalFeeRecipientsShares);
+
+ if (
+ totalFeeRecipientsSharesX18.add(ud60x18(share)).gt(
+ ud60x18(Constants.MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES)
+ )
+ ) {
+ revert Errors.FeeRecipientShareExceedsLimit();
+ }
+ }
​
// update protocol fee recipient
marketMakingEngineConfiguration.protocolFeeRecipients.set(feeRecipient, share);
​
// emit event LogConfigureFeeRecipient
emit LogConfigureFeeRecipient(feeRecipient, share);
}
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.