ComposableSecurity - The `_refundExcess` function does not work as whole `msg.value` is forwarder to `FeeManager`

[sherlock-admin4]

ComposableSecurity

medium

The _refundExcess function does not work as whole msg.value is forwarder to FeeManager

Summary

The _refundExcess function does not work, because the entire msg.value is forwarder to the FeeManager and there is never any Ether left on the Edition contract to be refunded.

Vulnerability Detail

The contract assumes the return of excess funds after collecting the appropriate fee. This is done by the _refundExcess function located in the Edition contract:

https://github.com/sherlock-audit/2024-04-titles/blob/d7f60952df22da00b772db5d3a8272a988546089/wallflower-contract-v2/src/editions/Edition.sol#L510-L517

This function refers to the balance of the Edition contract, but the entire msg.value is passed to FeeManager, so it is not possible for it to return the excess funds sent.

Not all users calculate the exact values when they are sending msg.value. Especially in batch transactions and when they are doing that through other contracts. As there exists the _refundExcess function it is assumed that the protocol want to protect users who do not send the exact values in msg.value.

POC results

Ran 1 test for test/editions/POC_M1_Edition.t.sol:EditionTest
[PASS] test_POC_user_funds_lost() (gas: 191539)
Logs:

  Before the mint
  Alice balance:  100000000000000000000
  FeeManager balance:  0
  Edition balance:  0

  After the mint
  Alice balance:  80000000000000000000
  FeeManager balance:  19989400000000000000
  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_user_funds_lost() public {

    // Scenario setup
    address alice = makeAddr("alice");
    vm.deal(alice, 100 ether);

    console.log("");
    console.log("Before the mint");
    console.log("Alice balance: ", alice.balance);
    console.log("FeeManager balance: ", address(feeManager).balance);
    console.log("Edition balance: ", address(edition).balance);
    console.log("");

    // Impersonate alice and mint 1 token.
    vm.startPrank(alice);
    edition.mint{value: 20 ether}(alice, 1, 1, address(0), new bytes(0));
    vm.stopPrank();

    console.log("");
    console.log("After the mint");
    console.log("Alice balance: ", alice.balance);
    console.log("FeeManager balance: ", address(feeManager).balance);
    console.log("Edition balance: ", address(edition).balance);
    console.log("");

    // Assertions to verify state changes
    assertEq(edition.balanceOf(alice, 1), 1, "User1 token balance incorrect");
}

}

Impact

Loss of the excessive Ether sent to the Edition contract.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/d7f60952df22da00b772db5d3a8272a988546089/wallflower-contract-v2/src/editions/Edition.sol#L510-L517

https://github.com/sherlock-audit/2024-04-titles/blob/d7f60952df22da00b772db5d3a8272a988546089/wallflower-contract-v2/src/editions/Edition.sol#L241 https://github.com/sherlock-audit/2024-04-titles/blob/d7f60952df22da00b772db5d3a8272a988546089/wallflower-contract-v2/src/editions/Edition.sol#L267 https://github.com/sherlock-audit/2024-04-titles/blob/d7f60952df22da00b772db5d3a8272a988546089/wallflower-contract-v2/src/editions/Edition.sol#L296 https://github.com/sherlock-audit/2024-04-titles/blob/d7f60952df22da00b772db5d3a8272a988546089/wallflower-contract-v2/src/editions/Edition.sol#L319

Tool used

Manual Review

Recommendation

Move _refundExcess() function to the FeeManager contract and use it on payer_ or do not send the entire msg.value, forward just calculated (e.g. through getMintFee) fees and return the rest to the user.

Duplicate of #269

[ComposableSecurityTeam]

Escalate

This is a valid issue. The duplicate was accepted and is rewarded here: https://github.com/sherlock-audit/2024-04-titles-judging/issues/269

[sherlock-admin3]

Escalate

This is a valid issue. The duplicate was accepted and is rewarded here: https://github.com/sherlock-audit/2024-04-titles-judging/issues/269

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]

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

[Evert0x]

Result: High Duplicate of #269

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ComposableSecurityTeam: accepted