WildcatMarket should not define feeRecipient immutable variable

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol#L33 https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarket.sol#L107

Vulnerability details

Impact

When a WildcatMarket is deployed by borrower via WildcatMarketController.deployMarket, feeRecipient is obtained by WildcatMarketControllerFactory.getProtocolFeeConfiguration() and assigned to immutable feeRecipient in the constructor of WildcatMarketBase. The variable is defined as immutable and therefore cannot be updated. All fees accumulated by a Market belong to feeRecipient.
We noticed that WildcatMarketControllerFactory.setProtocolFeeConfiguration can change the feeRecipient for subsequent deployment of WildcatMarket. However, all WildcatMarkets that have been created can only use the old feeRecipient. If the following happens:

• The current feeRecipient is lost by the protocol.
• The private key of the current feeRecipient was stolen by a hacker.
• If the asset is a token (USDT/USDC) with a blacklist mechanism, and the current feeRecipient is blacklisted.

Then, the protocol needs to set a new feeRecipient via setProtocolFeeConfiguration. However, the new feeRecipient will not affect those already deployed WildcatMarkets, which results in loss of fees from the protocol.

Proof of Concept

File: src\market\WildcatMarketBase.sol
32:   /// @dev Account that receives protocol fees.
33:->   address public immutable feeRecipient;
......
076:   constructor() {
077:     MarketParameters memory parameters = IWildcatMarketController(msg.sender).getMarketParameters();
......
120:->   feeRecipient = parameters.feeRecipient;
......
125:   }

L77, parameters are set before creating WildcatMarket, and parameters.feeRecipient is obtained through WildcatMarketControllerFactory.getProtocolFeeConfiguration().

L120, feeRecipient is set to parameters.feeRecipient. It can't changed due to immutable.

WildcatMarket will accumulate fees to the protocol over time, and anyone can call collectFees() to transfer asset to feeRecipient.

File: src\market\WildcatMarket.sol
096:   function collectFees() external nonReentrant {
097:     MarketState memory state = _getUpdatedState();
098:     if (state.accruedProtocolFees == 0) {
099:       revert NullFeeAmount();
100:     }
101:     uint128 withdrawableFees = state.withdrawableProtocolFees(totalAssets());
102:     if (withdrawableFees == 0) {
103:       revert InsufficientReservesForFeeWithdrawal();
104:     }
105:     state.accruedProtocolFees -= withdrawableFees;
106:     _writeState(state);
107:->   asset.safeTransfer(feeRecipient, withdrawableFees);
108:     emit FeesCollected(withdrawableFees);
109:   }

If feeRecipient is no longer available for the protocol, the asset accumulated to feeRecipient will always be stuck in the contract. Imagine if there would be hundreds or thousands of WildcatMarkets using the same feeRecipient, then this would be a significant amount of funding for the protocol.

By looking at the code of WildcatMarket and its parent class, feeRecipient is only used in collectFees(). This can completely remove feeRecipient and obtain the latest feeRecipient in this function. This will not bring any fee loss to the protocol.

Tools Used

Manual Review

Recommended Mitigation Steps

Add view function for controller:

File: src\WildcatMarketController.sol
517:   function getFeeRecipient() external view returns(address feeRecipient) {
518:     (
519:       feeRecipient,,,      
520:     ) = controllerFactory.getProtocolFeeConfiguration();
521:   }

then, modify the code of collectFees:

File: src\market\WildcatMarket.sol
096:   function collectFees() external nonReentrant {
.......
107:-    asset.safeTransfer(feeRecipient, withdrawableFees);
+        address rec = IWildcatMarketController(controller).getFeeRecipient();
+        require(rec != address(0x0), "NNN");
107:+    asset.safeTransfer(rec, withdrawableFees);
108:     emit FeesCollected(withdrawableFees);
109:   }

Assessed type

Context

[c4-pre-sort]

minhquanym marked the issue as low quality report

[minhquanym]

Seems intended

[c4-judge]

MarioPoneder marked the issue as unsatisfactory: Invalid

[securitygrid]

I think this intentional immutable feeRecipient needs to be paid attention to by sponsors. Otherwise, once the situation in the report occurs, it will cause a loss of protocol funds.

So please re-evaluate this issue. Thanks

[Tri-pathi]

Hi @MarioPoneder ,I see this as a valid issue. One of the primary use cases of protocol, as mentioned in the doc, is market makers as borrowers who wish to acquire a supply of a newly released token to facilitate trading, either via liquidity provision or operating in a central limit order book, i.e., for newly released tokens this could be a more centralization risk. They would love to restrict feeRecipient to be safe from fees for thousands of orders [ since the reputation of the token won't be an issue for them initially ] However, this protocol has a more significant impact since, due to its Under-collateralized nature, it promotes new assets to raise funds, i.e., the more considerable income of protocol(fee collection) is near to centralized assets. , and for traditional tokens, it's already an issue. This affects the primary revenue source of protocol, as mentioned in #618

[MarioPoneder]

Thank you for your comments!

Please also see the sponsor's comment on #397:

If blacklisting happens for a token like USDC or USDT, there are far wider implications at play - they have to go through WAVES of legal to do this, and being able to change the feeRecipient doesn't change the facts on the ground. Regarding the recipients potential compromise, this will be a multisig wallet anyway (not that that particular fact matters) - it'd be possible to collect fees and immediately route them to another address via a multicall in any event.

There is a workaround already - deploy a new controller with a different feeRecipient address and forego fees accrued for a given market.

The fee recipient is fully in the hands of the protocol owner while they are fully aware of the risk.
Nevertheless, I agree with grade C QA instead of invalidating the issue.

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-c