sherlock-audit / 2024-04-titles-judging

6 stars 6 forks source link

zoyi - Transferring ownership does not fully transfer ownership #302

Closed sherlock-admin3 closed 2 months ago

sherlock-admin3 commented 3 months ago

zoyi

medium

Transferring ownership does not fully transfer ownership

Summary

The old owner still receives minting proceeds even though the old owner has transferred the ownership.

Vulnerability Detail

As per the README.md:

Transfer full ownership of the work to a new address (`transferWork`). This is the only way to change the creator for a work.

A creator can call transferWork() to transfer full ownership to another address:

    function transferWork(address to_, uint256 tokenId_) external {
        Work storage work = works[tokenId_];
        if (msg.sender != work.creator) revert Unauthorized();
        work.creator = to_;
        emit WorkTransferred(address(this), tokenId_, to_);
    }

The problem is, even if a creator transfers full ownership to another address, the old creator will still receive the minting funds.

Proof of Concept

Run this test in Edition.t.sol:

    function test_poc_transfer_work() public {
        // Prank as the creator and transferWork to `address(2)`
        vm.prank(address(1));
        edition.transferWork(address(2), 1);

        // Ensure that the creator is now `address(2)`
        assertEq(edition.creator(1), address(2));

        // Mint twice and check if the proceeds go to the old creator, `address(1)`
        edition.mint{value: 0.0106 ether}(address(1), 1, 1, address(0), new bytes(0));
        edition.mint{value: 0.0106 ether}(address(1), 1, 1, address(0), new bytes(0));

        // Assert two mints
        assertEq(edition.totalSupply(1), 2);

        // Assert that the old creator received the minting proceeds
        assertEq(address(1).balance, 0.02 ether);
    }

This test will successfully run - the minting proceeds will go to the old creator even though the ownership has been fully transferred to a new address.

Impact

Loss of minting proceeds, which is significant. Imagine a big AI artist selling one of their works to a buyer. The big AI artist transfers ownership to the buyer but little does the buyer know the big AI artist will still be receiving newly minted proceeds.

Code Snippet

Edition.sol#L412-L420

Tool used

Manual Review

Recommendation

Route the minting proceeds to the creator, not the old creator.

Duplicate of #283

bronzepickaxe commented 2 months ago

Escalate

This is a dupe of https://github.com/sherlock-audit/2024-04-titles-judging/issues/283

sherlock-admin3 commented 2 months ago

Escalate

This is a dupe of https://github.com/sherlock-audit/2024-04-titles-judging/issues/283

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

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

WangSecurity commented 2 months ago

UPD: 283 is escalated and if it's invalid in the end, this escalation will be rejected cause it doesn't effect reward distribution.

Evert0x commented 2 months ago

Result: Medium Duplicate of #283

sherlock-admin4 commented 2 months ago

Escalations have been resolved successfully!

Escalation status: