QA Report

[code423n4]

Summary

We list 4 low-critical findings and 1 non-critical finding here:

• (Low) Lock pragma to ensure compiler version
• (Low) CoreCollection should use safe version of transferFrom for payableToken
• (Low) ProxyVault, CoreProxy, and SplitProxy is not upgradable
• (Low) RoyaltyVault inheritance order is not same as ProxyVault
• (Non) CoreFactory doesn't bind between projects and collections

In summary of recommended security practices, it's better to lock pragma version, use 3rd party libraries for security (e.g. SafeERC20), and follow openzeppelin document for upgradable contract deployment.

(Low) Lock pragma to ensure compiler version

Impact

Floating pragma may cause unexpected compilation time behaviour and introduce unintended bugs.

Proof of Concept

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L2
https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/ProxyVault.sol#L2
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/SplitProxy.sol#L2

Tools Used

hardhat

Recommended Mitigation Steps

Don't use ^, locking pragma to ensure compiler version. e.g. pragma solidity 0.8.4;

(Low) CoreCollection should use safe version of transferFrom for payableToken

Impact

Some ERC20 returns false rather than reverting when transfer fails, should use SafeERC20 to check return value.

Proof of Concept

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

Tools Used

hardhat

Recommended Mitigation Steps

Use SafeERC20: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol

    using SafeERC20 for IERC20;

(Low) ProxyVault, CoreProxy, and SplitProxy is not upgradable

Impact

In document: https://github.com/code-423n4/2022-03-joyn#smart-contracts

### CoreProxy.sol (37 sloc)

Contract used for deploying instances of CoreCollection.
We use the proxies for gas optimization and will allow us to upgrade our users contracts in the future.

### ProxyVault.sol (48 sloc)

Contract used for deploying instances of RoyaltyVault.
We use the proxies for gas optimization and will allow us to upgrade our users contracts in the future.

### SplitProxy.sol (101 sloc)

Contract used for deploying instances of Splitter.
We use the proxies for gas optimization and will allow us to upgrade our users contracts in the future.

But in source code, these contracts are not upgradable.

Proof of Concept

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreProxy.sol#L16
https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/ProxyVault.sol#L29
https://github.com/code-423n4/2022-03-joyn/blob/main/splits/contracts/SplitProxy.sol#L25

Tools Used

hardhat

Recommended Mitigation Steps

Add setter function to set _impl address, or follow openzeppelin documents (deploy proxies with hardhat): https://docs.openzeppelin.com/upgrades-plugins/1.x/hardhat-upgrades

(Low) RoyaltyVault inheritance order is not same as ProxyVault

Impact

Since IRoyaltyVault and ERC165 have no storage, Ownable storage follows right after VaultStorage in both RoyaltyVault and ProxyVault, so everything is fine at the moment. However, it is best to keep the ordering properly sorted out, else any future changes in inherited contracts might unexpectedly cause storage layout mismatch. This is especially crucial if the contracts are intended to be upgradeable as stated in the document.

Proof of Concept

https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/ProxyVault.sol#L8
https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L11

Tools Used

hardhat

Recommended Mitigation Steps

Keep the ordering properly sorted out.

(Non) CoreFactory doesn't bind between projects and collections

Impact

It doesn't have any structs/variables to record relationships between projects and collections. Thus any relationship will only be recorded in emitted events. We consider this behaviour unfavorable and urge developers to properly bind the collections to projects.

Proof of Concept

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

Tools Used

hardhat

Recommended Mitigation Steps

Define a mapping to bind projects and collections.