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

Incorrect implementation in configureFeeRecipient() can revert when updating an existing recipient’s shares

bajagasec

Summary

The configureFeeRecipient() function incorrectly validates fee recipient shares, causing it to revert when updating an existing recipient’s allocation. This prevents adjustments to fee recipient shares.

Vulnerability Details

The total fee recipient shares (totalFeeRecipientsShares) should never exceed MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES.
However, the validation in configureFeeRecipient() does not account for an existing share when computing the new total, leading to incorrect reverts.

if (share > 0) {
UD60x18 totalFeeRecipientsSharesX18 = ud60x18(marketMakingEngineConfiguration.totalFeeRecipientsShares);
​
if (
totalFeeRecipientsSharesX18.add(ud60x18(share)).gt(
ud60x18(Constants.MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES)
)
) {
revert Errors.FeeRecipientShareExceedsLimit();
}
}

In this implementation, share represents the new share being set for the recipient. However, the calculation does not take into account that the recipient may already have an existing share, leading to an incorrect comparison.

Example Scenario:

  • Assume there are two fee recipients, Alice and Bob, each with 25% shares.

  • The contract's maximum allowable shares (MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES) is 60%.

  • The owner wants to update Alice’s share from 25% to 20% or 25% to 30%.

  • When configureFeeRecipient() is called with share = 20%, the function incorrectly computes:

totalFeeRecipientsShares (50%) + new share (20%) > 60% (MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES)

  • This check fails to consider Alice’s existing 25% share, leading to an unnecessary revert.

Impact

  • Prevents valid updates to fee recipient allocations.

  • Limits protocol governance by disallowing share adjustments.

  • Could hinder fee recipient rebalancing, impacting protocol operations.

Tools Used

Manual Review

Recommendations

Modify configureFeeRecipient() to correctly account for the recipient’s existing share before performing the validation.

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); // Get oldFeeRecipientShares
​
+ if (share > oldFeeRecipientShares) { // Only validate if increasing the share
​
// 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();
}
​
// update protocol fee recipient
marketMakingEngineConfiguration.protocolFeeRecipients.set(feeRecipient, share);
​
// emit event LogConfigureFeeRecipient
emit LogConfigureFeeRecipient(feeRecipient, share);
}
Updates

Lead Judging Commences

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