Joyn contest - 0xkatana's results

1. Low - transfer return value is ignored

Impact

Some ERC20 tokens, such as USDT, don't revert when transfer/transferFrom fails. The transfer return value has to be checked (as there are some other tokens that returns false instead revert). safeTransfer should be used instead of transfer

Proof of Concept

safeTransferFrom should be used instead of transferFrom on this line https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L175

Tools Used

Manual analysis

Recommended Mitigation Steps

Use safeTransfer instead of transfer or check the return value of transfer. The return value of transfer is checked properly in these locations https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L44 https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L52 https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L237

2. Low - onlyUnInitialized modifier missing from initialize function

Impact

The CoreCollection initialize function is missing the onlyUnInitialized function. The onlyUnInitialized modifier is not used in the contract right now and this allows the initialize function to be called more than once.

Proof of Concept

The onlyUnInitialized modifier prevents functions from being run if the initialized is set to true https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L46

This modifier is missing from the initialize function https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L87

Tools Used

Manual analysis

Recommended Mitigation Steps

Add the onlyUnInitialized modifier to the initialize function

1. Use prefix not postfix in loops

Impact

Using a prefix increment (++i) instead of a postfix increment (i++) saves gas for each loop cycle and so can have a big gas impact when the loop executes on a large number of elements.

Proof of Concept

There is one example in CoreFactory https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreFactory.sol#L79 for (uint256 i; i < _collections.length; i++) {

Two examples in Splitter https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L50 https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L274

Tools Used

Manual analysis

Recommended Mitigation Steps

Use prefix not postfix to increment in a loop

2. Use calldata instead of memory for function parameters

Impact

Use calldata instead of memory for function parameters. Having function arguments use calldata instead of memory can save gas.

Proof of Concept

There are many cases of function arguments using memory instead of calldata https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreFactory.sol#L71-L72 https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreFactory.sol#L109-L110 https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreFactory.sol#L128 https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreFactory.sol#L142 https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L79-L81 https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L122-L123

Tools Used

Manual analysis

Recommended Mitigation Steps

Change function arguments from memory to calldata

3. Add payable to functions that won't receive ETH

Impact

Identifying a function as payable saves gas. Functions that have the onlyOwner modifier cannot be called by normal users and will not mistakenly receive ETH. These functions can be payable to save gas.

Proof of Concept

There are many functions that have the onlyOwner modifier and can be payable

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

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

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

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

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

Tools Used

Manual analysis

Recommended Mitigation Steps

Add payable to these functions for gas savings

4. Short require strings save gas

Impact

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

Proof of Concept

Several cases of this gas optimization were found. These are a few examples, but more may exist

1. https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L54
2. https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L146
3. https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L191
4. https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L206
5. https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L222

Tools Used

Manual analysis

Recommended Mitigation Steps

Shorten require strings

5. Remove unused code

Impact

The tokenExists modifier is never used in CoreCollection or elsewhere in Joyn. It can be removed for gas savings.

The onlyUnInitialized modifier is never used either, but onlyUnInitialized should be used on the initialize function, which is a separate issue

Proof of Concept

tokenExists is declared here but never used https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L59

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove unused code

6. Redundant zero initialization

Impact

Solidity does not recognize null as a value, so uint variables are initialized to zero. Setting a uint variable to zero is redundant and can waste gas.

Proof of Concept

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

uint256 amount = 0;

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the redundant zero initialization uint256 amount;

7. Use simple comparison

Impact

The comparison operators >= and <= use more gas than >, <, or ==. Replacing the >= and ≤ operators with a comparison operator that has an opcode in the EVM saves gas

Proof of Concept

This code is in verifyProof from Splitter is the same as an if/else statement https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/Splitter.sol#L277-L287

if (computedHash <= proofElement) {
    // Hash(current computed hash + current element of the proof)
    computedHash = keccak256(
        abi.encodePacked(computedHash, proofElement)
    );
} else {
    // Hash(current element of the proof + current computed hash)
    computedHash = keccak256(
        abi.encodePacked(proofElement, computedHash)
    );
}

A simple comparison can be used for gas savings by reversing the logic

if (computedHash > proofElement) {
    // Hash(current element of the proof + current computed hash)
    computedHash = keccak256(
        abi.encodePacked(proofElement, computedHash)
    );
} else {
    // Hash(current computed hash + current element of the proof)
    computedHash = keccak256(
        abi.encodePacked(computedHash, proofElement)
    );
}

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace the comparison operator and reverse the logic to save gas using the suggestions above

8. Public functions can be external

Impact

Declaring a function as external instead of public saves gas

Proof of Concept

Some functions can be declared external instead of public

• baseURI from CoreCollection
• incrementWindow from Splitter

Tools Used

Slither

Recommended Mitigation Steps

Change function from public to external to save gas

9. Remove unused variable

Impact

The depositedInWindow variable is declared but never used, wasting gas without a clear purpose

Proof of Concept

depositedInWindow is declared here but never used https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/SplitStorage.sol#L15

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove unused variable