Joyn contest - saian's results

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

Function Splitter:incrementWindow is executed by vault:sendToSplitter transfered tokens is pushed into balanceForWindow array and window is incremented, but the function can be executed by any contract and more than allowed share of tokens can be transfered by whitelisted users

Proof of concept bytes4 public constant IID_IROYALTY = type(IRoyaltyVault).interfaceId;

function incrementWindow(uint256 royaltyAmount) public returns (bool) { uint256 wethBalance;

require(
    IRoyaltyVault(msg.sender).supportsInterface(IID_IROYALTY),
    "Royalty Vault not supported"
);
require(
    IRoyaltyVault(msg.sender).getSplitter() == address(this),
    "Unauthorised to increment window"
);

wethBalance = IERC20(splitAsset).balanceOf(address(this));
require(wethBalance >= royaltyAmount, "Insufficient funds");

require(royaltyAmount > 0, "No additional funds for window");
balanceForWindow.push(royaltyAmount);
currentWindow += 1;
emit WindowIncremented(currentWindow, royaltyAmount);
return true;

} The function can be executed by any contract with function supportsInterface and getSplitter that returns splitter address

royaltyAmount equivalent to current unclaimed balance can be added to window and the window can be incremented to add windows

claimForAllWindows can be executed by whitelisted user to transfer percent allocated tokens from each new window

Mitigation A storage variable with tokens added to the window can be added, and total-windowBalance can be added to next window, and when token is transfered windowBalance can be decremented

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

Function CoreCollection:initialize can be executed by owner after initialisation and state variables like mintFee, maxSupply can be changed to increase/decrease fee and supply, isForSale can be set to false to stop token sale

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

Mitigation require statement can be added to revert if function is already initialized

QA Report

Low severity findings

token transfer in CoreCollection:withdraw will fail

In function CoreCollection mintfee is transfered to same address if vault is not initialized, these tokens can be withdrawn by owner using withdraw function, but transferFrom function will fail due to lack of allowance

Proof of concept

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

function withdraw() external onlyOwner {
    uint256 amount = payableToken.balanceOf(address(this));
    payableToken.transferFrom(address(this), msg.sender, amount); 
    emit NewWithdrawal(msg.sender, amount);
}

Mitigation

transferFrom can be replaced with transfer

Increment window and transfer funds

Function Splitter:incrementWindow is executed by vault:sendToSplitter transfered tokens is pushed into balanceForWindow array and window is incremented, but the function can be executed by any contract and more than allowed share of tokens can be transfered by whitelisted users

Proof of concept

bytes4 public constant IID_IROYALTY = type(IRoyaltyVault).interfaceId;

function incrementWindow(uint256 royaltyAmount) public returns (bool) {
    uint256 wethBalance;

    require(
        IRoyaltyVault(msg.sender).supportsInterface(IID_IROYALTY),
        "Royalty Vault not supported"
    );
    require(
        IRoyaltyVault(msg.sender).getSplitter() == address(this),
        "Unauthorised to increment window"
    );

    wethBalance = IERC20(splitAsset).balanceOf(address(this));
    require(wethBalance >= royaltyAmount, "Insufficient funds");

    require(royaltyAmount > 0, "No additional funds for window");
    balanceForWindow.push(royaltyAmount);
    currentWindow += 1;
    emit WindowIncremented(currentWindow, royaltyAmount);
    return true;
}

The function can be executed by any contract with function supportsInterface and getSplitter that returns splitter address

royaltyAmount equivalent to current unclaimed balance can be added to window and the window can be incremented to add windows

claimForAllWindows can be executed by whitelisted user to transfer percent allocated tokens from each new window

Mitigation

A storage variable with tokens added to the window can be added, and total-windowBalance can be added to next window, and when token is transfered windowBalance can be decremented

CoreCollection can be re-initialized by owner

Function CoreCollection:initialize can be executed by owner after initialisation and state variables like mintFee, maxSupply can be changed to increase/decrease fee and supply, isForSale can be set to false to stop token sale

Proof of concept

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

Mitigation

require statement can be added to revert if function is already initialized

Non-critical findings

Unused import

Imported file is not used in the contract and can be removed

Proof of concept

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

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

Mitigation

import statement can be removed

Lack of input validation

Impact

Input validation is absent for address variables which may result in re-deployment if address is wrong in constructor or value transfer to wrong address, it is recommended to add validation statements in all address inputs especially in value transfers and immutable variable initialisation.

Proof of concept

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

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

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/splits/contracts/SplitFactory.sol#L59-64

Mitigation

Add address validation statement

Lack of comments

Impact

Some functions in the code are not commented or missing parts of comments. Adding comments can improve readability of the code

Proof of concept

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

missing return comment https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/CoreFactory.sol#L108

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

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

missing param comment https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/splits/contracts/SplitFactory.sol#L57

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

Mitigation

Comments can be added to the functions

boolean comparison with constant

In function RoyaltyVault:sentToSplitter external function return value boolean is compared with boolean constant. The return value can be directly used in the require statement

Proof of concept

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

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

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

Mitigation

The return value of the function can be directly used in the require statement

Code Layout

Impact

Code can be structed in order type declarations, state variables, events and functions which can improve readability Refer: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#order-of-layout

Proof of concept

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

Gas Optimizations

1. Repeating code

Impact

In function CoreFactory:createProject, collection creation statements in for loop can be replaced by function addCollection

Proof of concept

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/CoreFactory.sol#L81-L91

for (uint256 i; i < _collections.length; i++) { 
    Collection memory _collection = _collections[i];
    address coreCollection = _createCollection(_collection);

    if (_collection.claimsMerkleRoot != bytes32(0)) {
    ICoreCollection(coreCollection).initializeClaims(
        _collection.claimsMerkleRoot
    );
    }

    emit NewCollection(_collection.id, coreCollection, _projectId);

    ICoreCollection(coreCollection).transferOwnership(msg.sender);
}

can be replaced by

    for (uint256 i; i < _collections.length; i++) { 
      Collection memory _collection = _collections[i];
      addCollection(_projectId, _collection);
    }

Mitigation

collection create statements inside for loop can be replaced by addCollection function

2. > can be replaced with !=

!= is more efficient than > for unsigned integers in require statements with optimizer enabled

Proof of concept

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

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

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

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

3. revert string can be reduced to 32 bytes

Impact

Revert strings take a min of 32 bytes, shortening revert strings to 32 bytes can decrease deployment cost as well as runtime cost when revert condition fails

Proof of concept

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Mitigation

reduce size of revert strings to 32 bytes or use custom errors

4. post-increment can be replaced with pre-increment

pre-increment costs less compared to post-increment, and unchecked can be added to the increment as there is no chance of overflow due to the condition statement

Proof of concept

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

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

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

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

Mitigation

Change post-increment to pre-increment

5. Function parameters can be changed to calldata

Function parameters that are read-only can be changed to calldata to avoid copying to memory and save gas

Proof of concept

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

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

Mitigation

Change parameters from memory to calldata

6. Avoid storage re-read and save gas

Storage variables read takes ~100 gas in repeated reads, these variables can be read once stored in local variables

Proof of concept

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

if (mintFee > 0) {
    _handlePayment(mintFee * amount); 
}

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

if (startingIndex == 0) {
    setStartingIndex();
}
tokenId = ((startingIndex + totalSupply()) % maxSupply) + 1; 
_mint(_to, tokenId);   

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

if (
    royaltyVault != address(0) &&
    IRoyaltyVault(royaltyVault).getVaultBalance() > 0 
) {
    IRoyaltyVault(royaltyVault).sendToSplitter();
}

splitterProxy, platformFeeRecepient, royaltyAsset in

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

        require(splitterProxy != address(0), "Splitter is not set");

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

        require(
            IERC20(royaltyAsset).transfer(splitterProxy, splitterShare) == true, 
            "Failed to transfer royalty Asset to splitter"
        );
        require(
            ISplitter(splitterProxy).incrementWindow(splitterShare) == true, 
            "Failed to increment splitter window"
        );
        require(
            IERC20(royaltyAsset).transfer(
                platformFeeRecipient,
                platformShare
            ) == true,
            "Failed to transfer royalty Asset to platform fee recipient"
        );

        emit RoyaltySentToSplitter(splitterProxy, splitterShare);
        emit FeeSentToPlatform(platformFeeRecipient, platformShare);

7. Function return value can be cached and re-used

When function is called multiple times in a same block, return value can be cached and re-used to save gas,

Proof of concept

royaltyVaultInitialized is called twice in _handlePayment

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

address recipient = royaltyVaultInitialized()
    ? royaltyVault
    : address(this);
payableToken.transferFrom(msg.sender, recipient, _amount);
emit NewPayment(msg.sender, recipient, _amount, royaltyVaultInitialized());

Mitigation

Store return value of royalVaultInitialized() and re-used

8. Re-order statement to save gas on revert

In Splitter:incrementWindow function royalAmount validation can be placed above to save gas if the call is reverted due to 0 value

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

    require(
        IRoyaltyVault(msg.sender).supportsInterface(IID_IROYALTY),
        "Royalty Vault not supported"
    );
    require(
        IRoyaltyVault(msg.sender).getSplitter() == address(this),
        "Unauthorised to increment window"
    );

    wethBalance = IERC20(splitAsset).balanceOf(address(this));
    require(wethBalance >= royaltyAmount, "Insufficient funds");

    require(royaltyAmount > 0, "No additional funds for window");
    balanceForWindow.push(royaltyAmount);
    currentWindow += 1;
    emit WindowIncremented(currentWindow, royaltyAmount);
    return true;

9. didSucceed in event TransferEth will always be true

In Splitter:transferSplitAsset event parameter didSucceed can be avoided as false value will be reverted

Proof of concept

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

    // Try to transfer ETH to the given recipient.
    didSucceed = IERC20(splitAsset).transfer(to, value);
    require(didSucceed, "Failed to transfer ETH");

    emit TransferETH(to, value, didSucceed); 

Mitigation

didSucceed can be removed

10. Unused private function attemptETHTransfer

In Splitter contract private function attemptEthTransfer is not called and can be removed to reduce bytecode

Proof of concept

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

Mitigation

The function can be removed