Joyn contest - leastwood's results

Lines of code

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

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/ERC721Payable.sol#L50-L56

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/royalty-vault/contracts/RoyaltyVault.sol#L43-L57

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/splits/contracts/Splitter.sol#L231-L240

Vulnerability details

Impact

The transfer() function is used to send royalty assets to the splitter contract and its recipients. If the vault operates on non-standard ERC20 tokens, its possible for transfers to not revert upon failure. Similarly, transferFrom() is used to pull funds from the user when minting an NFT in CoreCollection.mintToken(). The ERC721Payable._handlePayment() function does not check the return value of payableToken.transferFrom(). As a result, if this call does not revert and instead returns false upon a failed transfer, the mintToken() function will continue to execute successfully. Therefore, users may be able to mint NFTs for free.

Recommended Mitigation Steps

Consider integrating SafeERC20 in all areas of Joyn's code to ensure return values for transfer() and transferFrom() calls are handled correctly.

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#L112-L142

Vulnerability details

Impact

The incrementWindow() function is used to notify the splitter contract whenever funds are sent to the contract. Users who belong to the merkle tree are able to claim their set percentage allocation on each new window. However, it is possible to spoof a call to incrementWindow() such that the two requires are satisfied as follows:

• The contract returns true for IRoyaltyVault(msg.sender).supportsInterface(IID_IROYALTY)
• The contract returns the address of the splitter contract for IRoyaltyVault(msg.sender).getSplitter()

As a result, the current access control is insufficient to deter users from generating an infinite number of windows and claiming on those windows to drain the contract of its balance.

Recommended Mitigation Steps

Consider implementing proper access control such that msg.sender must be some preset RoyaltyVault.sol contract.

Lines of code

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

Vulnerability details

Impact

The initialize() function is called by CoreFactory.sol when creating projects or adding collections to an existing project. When ownership of the CoreCollection.sol contract is transferred to the project owner, it gives the owner access to a subset of functions, including the initialize() function.

This function does not implement proper access control to prevent reinitialization. As such, an owner could rug its own collection by reinitializing with a mint fee of zero and a different maxSupply, allowing them to potentially a duplicate NFT as the tokenId is calculated by the following equation:

• tokenId = ((startingIndex + totalSupply()) % maxSupply) + 1

Recommended Mitigation Steps

Prevent collection owners from reinitializing the CoreCollection.sol contract by making use of OpenZeppelin's Initializable.sol contract.

Lines of code

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

Vulnerability details

Impact

Joyn allows project owners to configure their own royalty vault or give the SplitFactory.sol contract the ability to do this when the Splitter.sol and RoyaltyVault.sol contracts are deployed. Because there is an option for the owner to control the royaltyVault address stored in CoreCollection.sol, it creates a degree of centralisation risk whereby the owner could break the functionality of _beforeTokenTransfer().

There are three key ways to break the functionality of _beforeTokenTransfer():

• Call setPlatformFee() and set it to some value greater than 10000. This will cause sendToSplitter() to revert.
• Call setPlatformFeeRecipient() and set it to the zero address such that transfers to that address will fail.
• If the custom royalty vault contract is upgradeable, we can upgrade the contract to a malicious implementation that contains logic forcing _beforeTokenTransfer() to always revert.

Proof of Concept

Let's consider the following scenario:

• The project owner configures their own upgradeable royalty vault by calling setRoyaltyVault().
• After all the NFTs have been minted by various users, the contract owner can upgrade the royalty vault contract such that calls to IRoyaltyVault(royaltyVault).getVaultBalance() and IRoyaltyVault(royaltyVault).sendToSplitter() revert.
• As a result, every time a user transfers their NFT, the transaction will revert as it attempts to send the vault's balance to the splitter contract.

Recommended Mitigation Steps

Consider preventing project owners from being able to call setRoyaltyVault(). This will help to reduce some of the centralisation risk that the protocol has.

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L139-L167 https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/ERC721Payable.sol#L50-L56

Vulnerability details

Impact

The mintToken() function is called to mint unique tokens from an ERC721 collection. This function will either require users to provide a merkle proof to claim an airdropped token or pay a fee in the form of a payableToken. However, because the payableToken is paid before a token is minted, it may be possible to reenter the mintToken() function if there is a callback attached before or after the token transfer. Because totalSupply() has not been updated for the new token, a user is able to bypass the totalSupply() + amount <= maxSupply check. As a result, if the user mints the last token, they can reenter and mint duplicate NFTs as the way tokenId is generated will wrap around to the start again.

Proof of Concept

For the sake of this example, let's say startingIndex = 0 and maxSupply = 100. tokenId is minted according to ((startingIndex + totalSupply()) % maxSupply) + 1. If we see that a user mints a token where totalSupply() = maxSupply - 1 = 99 and they reenter the function, then the next token to mint will actually be of index 1 as totalSupply() % maxSupply = 0. Calculating the first tokenId, we get ((0 + 0) % maxSupply) + 1 = 1 which is a duplicate of our example.

Recommended Mitigation Steps

Consider adding reentrancy protections to prevent users from abusing this behaviour. It may also be useful to follow the checks-effects pattern such that all external/state changing calls are made at the end.

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol

Vulnerability details

Impact

The Joyn documentation mentions that Joyn royalty vaults should be equipped to handle revenue generated on a collection's primary and secondary sales. Currently, CoreCollection.sol allows the collection owner to receive a fee on each token mint, however, there is no existing implementation which allows the owner of a collection to receive fees on secondary sales.

After discussion with the Joyn team, it appears that this will be gathered from Opensea which does not have an on-chain royalty mechanism. As such, each collection will need to be added manually on Opensea, introducing further centralisation risk. It is also possible for users to avoid paying the secondary fee by using other marketplaces such as Foundation.

Recommended Mitigation Steps

Consider implementing the necessary functionality to allow for the collection of fees through an on-chain mechanism. ERC2981 outlines the approiate behaviour for this.

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L31-L61 https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L149-L169

Vulnerability details

Impact

The RoyaltyVault.sol contract interacts with the Splitter.sol to send accumulated royalties to the collection's respective recipients. The sendToSplitter() function will query the balance of the royalty asset and send the amount after fee deductions to the splitter contract. However, the recipient of these funds expects the entire amount as Splitter.incrementWindow() is called on this amount. If fee-on-tokens or any rebasing token is used here, then the splitter contract will receive less than the sent amount. As a result, some users will not be able to claim their entire proportion of the royalties.

Recommended Mitigation Steps

Consider taking a snapshot of the token balance before and after the transfer and treat the difference between these two amounts as the received token amount. This will be compatible with all types of tokens and avoid any issues where users are unable to withdraw their entire deposited amount.

Whitelisted Users May Miss Out on Collection Airdrops

Lines of code

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

Vulnerability details

Impact

The Joyn protocol allows whitelisted users to claim NFTs under a token airdrop. The whitelisted user will call mintToken() with a specific amount and a valid merkle proof. In the event isForSale is set to true and some users have not claimed their airdrop, it is possible for all the available tokens to be bought by non-whitelisted users. As a result, any user who was meant to be able to claim an airdropped token with inevitably miss out on their opportunity as the collection is bought out before they can react.

Recommended Mitigation Steps

Consider allocating a portion of available NFTs to whitelisted users to prevent behaviour where users buy out a collection before airdropped users are able to claim their tokens.