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

The `configureFeeRecipient` function could be dosed.

kupiasec

Summary

In the configureFeeRecipient function, MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES check is incorrect.

Vulnerability Details

https://github.com/Cyfrin/2025-01-zaros-part-2/blob/main/src/market-making/branches/MarketMakingEngineConfigurationBranch.sol#L626

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 (
626: 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);
}

As we can see in the line 626, totalFeeRecipientsShares + share is used, instead of totalFeeRecipientsShares + share - oldFeeRecipientShares.
totalFeeRecipientsShares + share > totalFeeRecipientsShares + share - oldFeeRecipientShares.
As a result, if totalFeeRecipientsShares is almost same with MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES this function could be reverted when it shouldn't.

Impact

Owner may not set the feeRecipient and shares.

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);
// check if share is greater than zero to verify the total will not exceed the maximum shares
- if (share > 0) {
+ if (share > oldFeeRecipientShares) {
UD60x18 totalFeeRecipientsSharesX18 = ud60x18(marketMakingEngineConfiguration.totalFeeRecipientsShares);
​
if (
-626: totalFeeRecipientsSharesX18.add(ud60x18(share)).gt(
+626: totalFeeRecipientsSharesX18.add(ud60x18(share - oldFeeRecipientShares)).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 10 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.

Give us feedback!