Joyn contest - 0xDjango's results

Lines of code

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

Vulnerability details

Impact

ERC721Payable.sol and thus CoreCollection.sol contain a function called _handlePayment(). In this function, funds are transferred via payableToken.transferFrom() in order to mint an isForSale token. Many tokens do not revert on transfer failure and instead simply return false. Since payableToken can be any token used to receive payment, it is possible that users will be able to mint the ERC721 for free if it is set to one that doesn't revert automatically.

Proof of Concept

If the CoreCollection is initialized with a payableToken that does not revert on failure as is very common, the user will be able to call mint() with less than the necessary mintFee. Since the transfer of the ERC20 fails but the minting of the ERC721 succeeds, the user gets the NFT for free.

Tools Used

Manual Review

Recommended Mitigation Steps

Use SafeERC20.safeTransfer() whenever transferring ERC20 tokens. This function reverts on failure by default.

Lines of code

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

Vulnerability details

Impact

Owner can arbitrarily set platform fee without validation. If the fee is set too high, all token transfers to splitter will fail. E.g. platformFee > 10000

Proof of Concept

If the owner sets calls setPlatformFee(10001), all calls to sendToSplitter() will fail due to the following lines:

uint256 platformShare = (balanceOfVault * platformFee) / 10000;
uint256 splitterShare = balanceOfVault - platformShare;

Assuming that balanceOfVault contains even $1 USDC (1e6), the platformShare will be larger than balanceOfVault causing the function to revert.

Tools Used

Manual review.

Recommended Mitigation Steps

Add checks to ensure that platformFee <= 10000 to ensure continuous functionality. Most likely want this to be less than 1000 or similar.

Issue 1 (Low) - All function inputs should verify address != address(0)

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/CoreCollection.sol#L193 https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/splits/contracts/SplitFactory.sol#L60-L61 https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/royalty-vault/contracts/RoyaltyVault.sol#L76

There are several input addresses that should be verified != address(0). Most notably, the royaltyVault and platformFeeRecipient.

Issue 2 (Low) - platformFeeRecipient must be trusted

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

In the event that royaltyAsset gives control on transfer such as with an ERC777, a malicious platformFeeRecipient will have the power to stop all executions of sendToSplitter()

Issue 3 (Low) - Function definition doesn't match purpose

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

The transferSplitAsset() function contains comments about sending ETH, and the event that is emitted is called TransferETH despite only ERC20 transfers.

Issue 4 (Non-critical) - Private attemptETHTransfer() never used

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

Since attemptETHTransfer() is private and not used in the contract, it can safely be removed.

Issue 5 (non-critical) - Internal functions should start with underscore

Example: https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/MultiSigWallet.sol#L284

Code style best practice.

Issue 6 (non-critical) - Unnecessary bool in TransferETH event

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

The function reverts if transfer fails, so bool in event will always be true.