SeaDropMintPublicationAction._distributeFees may miscalculate the fee allocated to treasury in some cases

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/modules/act/seadrop/SeaDropMintPublicationAction.sol#L251

Vulnerability details

Impact

At the end of processPublicationAction, if expectedFees is greater than 0, _distributeFees will be called to distribute fees to referrers and treasury. The processActionParams.referrerProfileIds array stores the ProfileIds of all referrers, which are set by the caller. If the array is empty, the fee originally belonging to referrers should not be deducted from feesToDistribute, and all feesToDistribute should be distributed to treasury.

Proof of Concept

The flow of executing SeaDropMintPublicationAction via LensHub.act is as follows:

LensHub.act //PublicationActionParams.actionModuleAddress is set to SeaDropMintPublicationAction
  ActionLib.act
    _isActionEnabled(_actedOnPublication, actionModuleId)   //check whether actionModuleAddress is enabled.
    ValidationLib.validateReferrersAndGetReferrersPubTypes  //publicationActionParams.referrerProfileIds can be empty array
    IPublicationActionModule(actionModuleAddress).processPublicationAction
      //check mint price
      _validateFeesAndRescaleThemIfNecessary
      //perform the mint payment from the transaction executor
      SEADROP.mintPublic
      _distributeFees   //expectedFees > 0

Let's look at the code snippet of _distributeFees:

File: contracts\modules\act\seadrop\SeaDropMintPublicationAction.sol
219:     function _distributeFees(
220:         uint256 feesToDistribute,
221:         uint256 mintPaymentAmount,
222:         address lensTreasuryAddress,
223:         CollectionData memory collectionData,
224:         Types.ProcessActionParams calldata processActionParams
225:     ) internal {
226:         // Wrap MATIC back into WMATIC.
227:         WMATIC.deposit{value: feesToDistribute}();
228:         
229:->       uint256 referrersCut = (mintPaymentAmount * collectionData.referrersFeeBps) / MAX_BPS;
230: 
231:         uint256 referrersQuantity = processActionParams.referrerProfileIds.length;
......
246:         // Because we already know that
247:         //     `publicDrop.feeBps >= lensTreasuryFeeBps + collectionData.referrersFeeBps`
248:         // then
249:         //     `feesToDistribute - referrersCut`
250:         // will be the Lens Treasury Fee plus any fee excess.
251:->       uint256 lensTreasuryCutPlusExcess = feesToDistribute - referrersCut;
252:         if (lensTreasuryCutPlusExcess > 0) {
253:             WMATIC.transfer(lensTreasuryAddress, lensTreasuryCutPlusExcess);
254:         }
255:     }

It can be seen from L229 that referrersCut is the fee to be distributed to processActionParams.referrerProfileIds. If referrersCut is 0, there is no problem. Its value is determined by collectionData.referrersFeeBps, which is assigned in initialization. Here, we are discussing the case where referrersCut is greater than 0.

If processActionParams.referrerProfileIds is an empty array, it means that all feesToDistribute should be distributed to treasury. However, L251 did not check this case, then directly deducted referrersCut.

Tools Used

Manual Review

Recommended Mitigation Steps

File: contracts\modules\act\seadrop\SeaDropMintPublicationAction.sol
219:     function _distributeFees(
220:         uint256 feesToDistribute,
221:         uint256 mintPaymentAmount,
222:         address lensTreasuryAddress,
223:         CollectionData memory collectionData,
224:         Types.ProcessActionParams calldata processActionParams
225:     ) internal {
226:         // Wrap MATIC back into WMATIC.
227:         WMATIC.deposit{value: feesToDistribute}();
228:         
229:         uint256 referrersCut = (mintPaymentAmount * collectionData.referrersFeeBps) / MAX_BPS;
230: 
231:         uint256 referrersQuantity = processActionParams.referrerProfileIds.length;
......
250:         // will be the Lens Treasury Fee plus any fee excess.
251:---      uint256 lensTreasuryCutPlusExcess = feesToDistribute - referrersCut;
251:+++      uint256 lensTreasuryCutPlusExcess = referrersQuantity > 0 ? (feesToDistribute - referrersCut) : feesToDistribute;
252:         if (lensTreasuryCutPlusExcess > 0) {
253:             WMATIC.transfer(lensTreasuryAddress, lensTreasuryCutPlusExcess);
254:         }

Assessed type

Other

[141345]

same root as https://github.com/code-423n4/2023-07-lens-findings/issues/97, maybe combine

[donosonaumczuk]

This haven't been tested yet, this modules contracts are out of scope, specified in the provided documentation. We kept them in the repo for context, so wardens can understand how the flows work with proper examples.

We appreciate the finding.

[c4-sponsor]

donosonaumczuk marked the issue as sponsor disputed

[c4-judge]

Picodes marked the issue as unsatisfactory: Out of scope