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

`MarketMakingEngineConfigurationBranch.sol#configureFeeRecipient()` checks if result total share will not exceed maximum shares as wrong way.

danielarmstrong

Summary

MarketMakingEngineConfigurationBranch.sol#configureFeeRecipient() checks if result total share will not exceed maximum shares as wrong way.

Vulnerability Details

MarketMakingEngineConfigurationBranch.sol#configureFeeRecipient() function is as follows.

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();
}
​
// update protocol fee recipient
marketMakingEngineConfiguration.protocolFeeRecipients.set(feeRecipient, share);
​
// emit event LogConfigureFeeRecipient
emit LogConfigureFeeRecipient(feeRecipient, share);
}

As we can see above, the check of maximum share does not consider oldFeeRecipientShares.

Impact

This vulnerability will cause that owner cannot configure share even if total share does not exceed maximum share.

Tools Used

Manual review

Recommendations

Modify MarketMakingEngineConfigurationBranch.sol#configureFeeRecipient() as follows.

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 > oldFeeRecipientShares) {
++ UD60x18 totalFeeRecipientsSharesX18 = ud60x18(marketMakingEngineConfiguration.totalFeeRecipientsShares);
++
++ if (
++ totalFeeRecipientsSharesX18.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 5 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.