Summary
When the total fee recipient shares reach the maximum limit, reducing a recipient’s share is blocked due to a validation check. This prevents owners from updating feeBps for existing recipients.
Vulnerability Details
The configureFeeRecipient allows owner to add , remove or update shares of recipient’s . when we calls this function it first validate that the newShare value will not exceeds the max limit of allows recipients’s shares.
/home/aman/Desktop/audits/2025-01-zaros-part-2/src/market-making/branches/MarketMakingEngineConfigurationBranch.sol:613
613: function configureFeeRecipient(address feeRecipient, uint256 share) external onlyOwner {
614:
615: if (feeRecipient == address(0)) revert Errors.ZeroInput("feeRecipient");
616:
617:
618: MarketMakingEngineConfiguration.Data storage marketMakingEngineConfiguration =
619: MarketMakingEngineConfiguration.load();
620:
621:
622: if (share > 0) {
623: UD60x18 totalFeeRecipientsSharesX18 = ud60x18(marketMakingEngineConfiguration.totalFeeRecipientsShares);
624:
625: if (
626: totalFeeRecipientsSharesX18.add(ud60x18(share)).gt(
627: ud60x18(Constants.MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES)
628: )
629: ) {
630: revert Errors.FeeRecipientShareExceedsLimit();
631: }
632: }
633:
634: (, uint256 oldFeeRecipientShares) = marketMakingEngineConfiguration.protocolFeeRecipients.tryGet(feeRecipient);
635:
636:
637: if (oldFeeRecipientShares > 0) {
638: if (oldFeeRecipientShares > share) {
639: marketMakingEngineConfiguration.totalFeeRecipientsShares -=
640: (oldFeeRecipientShares - share).toUint128();
641: } else {
642: marketMakingEngineConfiguration.totalFeeRecipientsShares +=
643: (share - oldFeeRecipientShares).toUint128();
644: }
645: } else {
646: marketMakingEngineConfiguration.totalFeeRecipientsShares += share.toUint128();
647: }
648:
649:
650: marketMakingEngineConfiguration.protocolFeeRecipients.set(feeRecipient, share);
651:
652:
653: emit LogConfigureFeeRecipient(feeRecipient, share);
654: }
The above code will not work as intended , as in case if the totalFeeRecipientsSharesX18=MAX_CONFIGURABLE_PROTOCOL_FEE_SHARES and owner wants to decrease share of specfic recipient’s it will always revert due to check before removal of shares. The following POC will demonstrates it.
POC
/test/integration/market-making/market-making-engine-configuration-branch/configureFeeRecipient/configureFeeRecipieint.t.sol:39
39: function test_totalFeeRecipientsShare_is_max_poc() external {
40: address user1 = address(0x1234);
41: address user2 = address(0x5678);
42: marketMakingEngine.configureFeeRecipient(user1, 0.8e18);
43: marketMakingEngine.configureFeeRecipient(user2, 0.1e18);
44:
45:
46: marketMakingEngine.configureFeeRecipient(user1, 0.7e18);
47: }
run the Test with Command : forge test --mt test_totalFeeRecipientsShare_is_max_poc
Impact
Owners cannot reduce a recipient’s feeBps when the total fee recipients’ shares are at the maximum limit.
Tools Used
Manual Review
Recommendations
Modify the configureFeeRecipient function to allow fee share reductions even when the total shares are at the max limit. Proposed Fix:
@@ -621,17 +621,7 @@ contract MarketMakingEngineConfigurationBranch is OwnableUpgradeable {
// check if share is greater than zero to verify the total will not exceed the maximum shares
// @audit : what if share is already on 0.9e18 limit , and we want to decrease share of a user how can we do that ?
- 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);
@@ -649,7 +639,17 @@ contract MarketMakingEngineConfigurationBranch is OwnableUpgradeable {
} else {
marketMakingEngineConfiguration.totalFeeRecipientsShares += share.toUint128();
}
+ if (share > 0) {
+ 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);
