sherlock-audit / 2024-04-titles-judging

9 stars 6 forks source link

brakeless - Collection referrers do not receive their allocation of the protocolShare #217

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 4 months ago

brakeless

medium

Collection referrers do not receive their allocation of the protocolShare

Summary

When the protocolShare is split in FeeManager::_splitProtocolFees(), the collectionReferrerShare is mistakenly sent to the mint referrer_.

Vulnerability Detail

The collectionReferrerShare is checked against referrers[edition_], but at the end of the function the fee is routed to referrer_.

The referrer_, which represents the mint referrer, receives two payments - both mintReferrerShare and collectionReferrerShare.

function _splitProtocolFee(
    IEdition edition_,
    address asset_,
    uint256 amount_,
    address payer_,
    address referrer_
) internal returns (uint256 referrerShare) {
    // The creation and mint referrers earn 25% and 50% of the protocol's share respectively, if applicable  
    uint256 mintReferrerShare = getMintReferrerShare(amount_, referrer_);
    uint256 collectionReferrerShare = getCollectionReferrerShare(amount_, referrers[edition_]);  // @audit collection referrer checked here
    referrerShare = mintReferrerShare + collectionReferrerShare;

    _route(
        Fee({asset: asset_, amount: amount_ - referrerShare}),
        Target({target: protocolFeeReceiver, chainId: block.chainid}),
        payer_
    );

    _route(
        Fee({asset: asset_, amount: mintReferrerShare}),
        Target({target: referrer_, chainId: block.chainid}),
        payer_
    );

    _route(
        Fee({asset: asset_, amount: collectionReferrerShare}),   
        Target({target: referrer_, chainId: block.chainid}),     // @audit shouldn't target here be referrers[edition_] ? 
        payer_                                                  
    );
}

Add this test to FeeManager.t.sol.

Run with the following command: forge test --match-test test_collectMintFeeWithCollectionReferrer -vvvv

function test_collectMintFeeWithCollectionReferrer() public {
    address mintReferrer = address(0x1234);
    address collectionReferrer = address(0x1337);

    FeeManager fManager = new FeeManager(address(1), address(this), address(new MockSplitFactory()));

    feeManager.createRoute(IEdition(address(mockEdition)), 1, new Target[](0), collectionReferrer);

    console.log('mintReferrer balance BEFORE collectMintFee(): %d', mintReferrer.balance);
    console.log('collectionReferrer balance BEFORE collectMintFee(): %d', collectionReferrer.balance);

    feeManager.collectMintFee{value: 0.0106 ether}(
        IEdition(address(mockEdition)), 1, 1, address(this), mintReferrer
    );

    console.log('\n  mintReferrer balance AFTER collectMintFee(): %d', mintReferrer.balance);
    console.log('collectionReferrer balance AFTER collectMintFee(): %d', collectionReferrer.balance);

    /*
    protocolShare = 0.0006 ether
    -> mintReferrerShare = 0.0006 ether * 5000 / 10000 = 300000000000000
    -> collectionReferrerShare = 0.0006 ether * 2500 / 10000 = 150000000000000
    -> protocol allocation = remaining amount = 150000000000000
     */
}
[PASS] test_collectMintFeeWithCollectionReferrer() (gas: 2276379)
Logs:
  mintReferrer balance BEFORE collectMintFee(): 0
  collectionReferrer balance BEFORE collectMintFee(): 0

  mintReferrer balance AFTER collectMintFee(): 450000000000000
  collectionReferrer balance AFTER collectMintFee(): 0

As shown in the output, the mint referrer collects all the referrer fees and the collection referrer receives nothing.

Impact

Loss of fees for collection referrers

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L412-L441

Tool used

Manual Review

Recommendation

Change the target destination to referrers[edition_] for the collectionReferrerShare route.

 function _splitProtocolFee(
     IEdition edition_,
     address asset_,
     uint256 amount_,
     address payer_,
     address referrer_
 ) internal returns (uint256 referrerShare) {
     // SNIP

     _route(
         Fee({asset: asset_, amount: collectionReferrerShare}),   
-        Target({target: referrer_, chainId: block.chainid}), 
+        Target({target: referrers[edition_], chainId: block.chainid})
         payer_                                                  
     );
 }

Duplicate of #267

sherlock-admin3 commented 3 months ago

Escalate

Requesting another review of this report. It can be argued that this report is a duplicate of #267 - Same root cause and impact.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

brakeless-wtp commented 3 months ago

Escalate

Requesting another review of this report. It can be argued that this report is a duplicate of #267 - Same root cause and impact.

sherlock-admin3 commented 3 months ago

Escalate

Requesting another review of this report. It can be argued that this report is a duplicate of #267 - Same root cause and impact.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

WangSecurity commented 3 months ago

Agree with the escalation, planning to accept and duplicate with #267

Evert0x commented 3 months ago

Result: High Duplicate of #267

sherlock-admin4 commented 3 months ago

Escalations have been resolved successfully!

Escalation status: