Joyn contest - hubble's results

Lines of code

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

Vulnerability details

The initialize function in CoreCollection.sol is not protected properly and can be called by the Owner multiple times. The modifier onlyUnInitialized() is missing in the intialize() function.

Impact

If by mistake wrong/different values of maxSupply, mintFee, or payableToken is set again in the initialize(), then the protocol state will become inconsistent, and any subsequent operations on the contract may cause loss of value depending on the differences in the new values set.

Proof of Concept

Contract CoreCollection.sol
Function initialize()

Recommended Mitigation Steps

Add the modifier onlyUnInitialized() in the initialize() function of the CoreCollection.sol contract

Lines of code

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

Vulnerability details

Impact

(at RoyaltyVault.sol) Presently platformFee, does not have a upper limit and can be set to any value through setPlatformFee function. If the value is set beyond 10,000 it would cause an underflow during split share calculation at sendToSplitter function.

Proof of Concept

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

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

Tools Used

Manual review

Recommended Mitigation Steps

• Decide on a maximum limit to platformFee (%)
• Validate that with a require statement at setPlatformFee function

Lines of code

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

Vulnerability details

The below transferFrom command is called at two places in the core contracts, followed by an emit event

payableToken.transferFrom(msg.sender,recipient,_amount)
emit ...(...);

The return value is not checked during the payableToken.transferFrom

Impact

In the event of failure of payableToken.transferFrom(...), the emit event is still generated causing the downstream applications to capture wrong transaction / state of the protocol.

Proof of Concept

1. Contract CoreCollection.sol
   function withdraw()

2. Contract ERC721Payable.sol function _handlePayment

Recommended Mitigation Steps

Add a require statement as being used in the RoyaltyVault.sol

require( payableToken.transferFrom(msg.sender,recipient,_amount) == true,
            "Failed to transfer amount to recipient" );

Summary of Findings for Low / Non-Critical issues

Issue#1 : Null address input check missing in few functions Issue#2 : No check on upper bound value of platformFee in function setPlatformFee Issue#3 : Unused state variable HASHED_PROOF in CoreCollection.sol Issue#4 : Check for getVaultBalance() is redundant in Contract CoreCollection.sol Issue#5 : Solidity Pragma not fixed to a particular version

Details Issue#1

Title : Null address input check missing in few functions

Listed below few functions with missing check.

a. Contract CoreCollection.sol, function setRoyaltyVault(address _royaltyVault) b. Contract RoyaltyVault.sol , function setPlatformFeeRecipient (address _platformFeeRecipient)

Recommended Mitigation Steps

Add a require statement to check for null address for the input address parameters

Details Issue#2

Title : No check on upper bound value of platformFee in function setPlatformFee

In contract RoyaltyVault.sol

   function setPlatformFee(uint256 _platformFee) external override onlyOwner {
        platformFee = _platformFee;
        emit NewRoyaltyVaultPlatformFee(_platformFee);
    }

There is no check on the upper bound value, and in case of wrong value set, the platformShare and splitterShare values calculated will be affected.

Recommended Mitigation Steps

Add a require statement to check for a max value defined.

Details Issue#3

Title : Unused state variable HASHED_PROOF in CoreCollection.sol

The state variable HASHED_PROOF is defined in the CoreCollection.sol contract and its value is also being set by the function setHashedProof(). However this variable does not seem to be used in any of the contracts.

Recommended Mitigation Steps

Remove the unwanted dead code.

Details Issue#4

Title : Check for getVaultBalance() is redundant in Contract CoreCollection.sol

Contract : CoreCollection.sol function _beforeTokenTransfer(...) Line 305 : IRoyaltyVault(royaltyVault).getVaultBalance() > 0

This check is redundant because, this value for getVaultBalance is again checked in the subsequent call in IRoyaltyVault(royaltyVault).sendToSplitter()

Recommended Mitigation Steps

Remove the redundant check from the _beforeTokenTransfer(...) function

Details Issue#5

Title : Solidity Pragma not fixed to a particular version

Almost all of the contracts are using a floating pragma definition, which is not as per best practice.

  pragma solidity ^0.8.0;

Recommended Mitigation Steps

Decide on a particular version say 0.8.10 and hardcode it in all the contracts.

  pragma solidity 0.8.10;