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 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.