SimpleFeeCollectModule/MultirecipientFeeCollectModule should be able to modify the recipient receiving the fee

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/modules/act/collect/base/BaseFeeCollectModule.sol#L249 https://github.com/code-423n4/2023-07-lens/blob/main/contracts/modules/act/collect/MultirecipientFeeCollectModule.sol#L176-L195

Vulnerability details

Impact

As the sponsor confirmed, they will support USDT/USDC with a blacklist mechanism as a whitelist token. When the fee recipient is blacklisted, transferring to the blacklisted address will cause the entire tx to revert. There are two modules in the codebase that will be affected: SimpleFeeCollectModule, MultirecipientFeeCollectModule. The case discussed here is: when the fee recipient is blacklisted is not during initialization but after initialization.

Proof of Concept

Both SimpleFeeCollectModule and MultirecipientFeeCollectModule inherit from BaseFeeCollectModule. SimpleFeeCollectModule can initialize a single fee recipient, while MultirecipientFeeCollectModule can initialize multiple fee recipients. However, none of these two modules can update the fee recipient after initialization.

When a user collects one publication via LensHub.act, the flow is as follows:

LensHub.act
  ActionLib.act
    IPublicationActionModule(actionModuleAddress).processPublicationAction  //actionModuleAddress=CollectPublicationAction
      CollectPublicationAction._processCollect(collectModule, processActionParams)
        ICollectModule(collectModule).processCollect  //BaseFeeCollectModule is base contract.

The code of BaseFeeCollectModule.processCollect is as follows:

File: contracts\modules\act\collect\base\BaseFeeCollectModule.sol
54:     function processCollect(Types.ProcessCollectParams calldata processCollectParams)
55:         external
56:         virtual
57:         onlyActionModule
58:         returns (bytes memory)
59:     {
60:         _validateAndStoreCollect(processCollectParams);
61: 
62:         if (processCollectParams.referrerProfileIds.length == 0) {
63:             _processCollect(processCollectParams);
64:         } else {
65:             _processCollectWithReferral(processCollectParams);
66:         }
67:         return '';
68:     }

_processCollect/_processCollectWithReferral will subcall _transferToRecipients internally.

File: contracts\modules\act\collect\base\BaseFeeCollectModule.sol
239:     function _transferToRecipients(
240:         Types.ProcessCollectParams calldata processCollectParams,
241:         address currency,
242:         uint256 amount
243:     ) internal virtual {
244:         address recipient = _dataByPublicationByProfile[processCollectParams.publicationCollectedProfileId][
245:             processCollectParams.publicationCollectedId
246:         ].recipient;
247: 
248:         if (amount > 0) {
249:->           IERC20(currency).safeTransferFrom(processCollectParams.transactionExecutor, recipient, amount);
250:         }
251:     }

If the recipient is blacklisted, then L249 will revert. Because SimpleFeeCollectModule does not override _transferToRecipients, it uses the above code. MultirecipientFeeCollectModule overrides the function, the code is as follows:

File: contracts\modules\act\collect\MultirecipientFeeCollectModule.sol
165:     function _transferToRecipients(
166:         Types.ProcessCollectParams calldata processCollectParams,
167:         address currency,
168:         uint256 amount
169:     ) internal override {
170:         RecipientData[] memory recipients = _recipientsByPublicationByProfile[
171:             processCollectParams.publicationCollectedProfileId
172:         ][processCollectParams.publicationCollectedId];
173:         uint256 len = recipients.length;
174: 
175:         // If only 1 recipient, transfer full amount and skip split calculations
176:         if (len == 1) {
177:->           IERC20(currency).safeTransferFrom(
178:                 processCollectParams.transactionExecutor,
179:                 recipients[0].recipient,
180:                 amount
181:             );
182:         } else {
183:             uint256 i;
184:             while (i < len) {
185:                 uint256 amountForRecipient = (amount * recipients[i].split) / BPS_MAX;
186:                 if (amountForRecipient != 0)
187:->                   IERC20(currency).safeTransferFrom(
188:                         processCollectParams.transactionExecutor,
189:                         recipients[i].recipient,
190:                         amountForRecipient
191:                     );
192:                 unchecked {
193:                     ++i;
194:                 }
195:             }
196:         }
197:     }

Any recipient is blacklisted in the recipients array, tx will revert.

As mentioned above, if the fee recipient specified by a publication is blacklisted, no one can collect the publication anymore.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to add a function for the owner of the publication to update the fee recipient.

Assessed type

DoS

[donosonaumczuk]

• Module contracts are out of scope
• It is an interesting feature, but also has issues like "Collect this, funds go to charity" and then you change it to go to yourself

[c4-sponsor]

donosonaumczuk marked the issue as sponsor disputed

[c4-judge]

Picodes marked the issue as unsatisfactory: Out of scope