Joyn contest - Ruhum's results

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L149-L169 https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L35

Vulnerability details

Vulnerability details

Impact

The Splitter contract is used to split up the creator's proceeds. By repeatedly calling the incrementWindow() function, a creator can steal the other's share of the proceeds.

Proof of Concept

Let's say the currentWindow is at 0.

The Splitter contract received 5e18 tokens that should be distributed between two parties. Both get 50% of the pot.

Alice creates a contract that fulfills the two require statements here: https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L152-L159

After the contract got the tokens, Alice calls the incrementWindow() function using her contract twice. Creating two windows each with a pot of 5e18: https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L149-L169

After that, Alice claims both windows using claimForAllWindows(): https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L35

For both windows, she receives 50% of the window's pot. That's 2 * 0.5 * 5e18 == 5e18. Thus, she takes all the funds of the Splitter contract. There's nothing left for Bob the other creator.

Tools Used

none

Recommended Mitigation Steps

Store the balance at the time of the window creation in a state variable. When increasing the window, only assign the difference between the new and the old balance to the window's pot. Otherwise, someone can create multiple windows with the same balance.

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L35-L62 https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L149-L169

Vulnerability details

Impact

This is somewhat related to my other submission: "Creator can steal other creator's split in Splitter contract".

But, now instead of a creator stealing the other's funds, the issue is that someone else can DOS the usage of claimForAllWindows. Since anybody is able to call incrementWindow() they can create so many windows that any calls to claimForAllWindows after that will always fail.

Proof of Concept

An attacker implements a contract that calls the incrementWindow() function: https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L149-L169 The contract they use implements the necessary functions to pass the two require functions.

They use the contract to call the incrementWindow() function a larger number of times. After that, calls to the claimForAllWindows() function will result in a way too high amount value. A value that is higher than the contract's actual balance. Thus, any calls to the function will revert: https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L61

Tools Used

none

Recommended Mitigation Steps

Limit the access to the incrementWindow() function.

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L78-L97

Vulnerability details

Impact

The CoreCollection.initialize() function can be called multiple times by the creator of the collection. It is used to configure the collection. A malicious owner can abuse that by retroactively changing certain configurations. They could increase the max supply of the collection. Or, they could frontrun a user's purchase and increase the mintFee draining the user's funds. They can also change the splitFactory value which is normally not configurable by the owner.

Proof of Concept

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L78-L97

    function initialize(
        string memory _collectionName,
        string memory _collectionSymbol,
        string memory _collectionURI,
        uint256 _maxSupply,
        uint256 _mintFee,
        address _payableToken,
        bool _isForSale,
        address _splitFactory
    ) external onlyOwner onlyValidSupply(_maxSupply) {
        _name = _collectionName;
        _symbol = _collectionSymbol;
        _baseUri = _collectionURI;
        maxSupply = _maxSupply;
        mintFee = _mintFee;
        payableToken = IERC20(_payableToken);
        isForSale = _isForSale;
        splitFactory = _splitFactory;
        initialized = true;
    }

The initialize() function only has two modifiers: onlyOwner() and onlyValidSupply(). The first prevents anybody but the owner from calling this function. The other enforces the maxSupply value to be > 0. There is no check whether the collection was already initialized. The first initialization is done by the CoreFactory contract here. After that, the ownership is transferred to the original caller here. Thus, the creator is able to call the initialize() function multiple times.

Now let's say a user wants to mint 5 NFTs. The initial mintFee is 1e18. For whatever reason the user approves more funds to the collection than necessary for that purchase. Let's say they approve 10e18. Maybe they want to buy more in another transaction. Who knows. The creator of the NFT collection searches the mempool for a situation like this. It frontruns the user's transaction and calls initialize() to set the mintFee to 2e18 instead. Now they can pull the whole 10e18 tokens from the user's wallet here.

Tools Used

none

Recommended Mitigation Steps

Add the onlyUnInitialized() modifier to the initialize() function.

Judge has assessed an item in Issue #25 as High risk. The relevant finding follows:

Fees should have a boundary of 100% (10000): https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L68

Otherwise the contract will try to transfer more than possible which will result in reverts: https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L40

It might also be helpful the have an fixed upper boundary that doesn't allow the platform to collect more than a set amount of fees, e.g. 10%.

Report

Don't ignore ERC20 transfer return values

You're ignoring the return value of an ERC20 transfer twice:

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L175

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/ERC721Payable.sol#L54

Either use SafeERC20 or check the return value as you do in other places in the code base.

Add a max boundary for the platform fee

Fees should have a boundary of 100% (10000): https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L68

Otherwise the contract will try to transfer more than possible which will result in reverts: https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L40

It might also be helpful the have an fixed upper boundary that doesn't allow the platform to collect more than a set amount of fees, e.g. 10%.

No connection between a project & collection

When creating a project with collections, there is no ID linking the collections to that specific project. Both entities exist independent of each other. There seems to be no way of associating them with each other after the creation (besides the emitted event).

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreFactory.sol#L70-L99