Joyn contest - rayn's results

Lines of code

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

Vulnerability details

Impact

Splitter.incrementWindow checks incrementation of royalty against the entire balance of SplitProxy. Malicious users can thus re-count any not yet withdrawn splitAsset and include in a new window, and subsequently withdraw more shares than allowed, thus stealing earnings from other users.

Proof of Concept

Splitter.incrementWindow should check window incrementation against newly transferred royalty, and reject if the amount is larger than the received amount of tokens. However, the current implementation accidentally checks against the entire balance, thus if any not yet withdrawn tokens exist, malicious users can re-count those tokens, and immediately withdraw a share from those.

        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");

To illustrate the attack more clearly, suppose

1. There are currently 6.25 splitAsset tokens in SplitProxy
2. The attacker is entitled with 1/5 of the royalties
3. No user has withdrawn any royalty yet.

The attack can be executed as follow

1. Attacker first withdraws his share of royalty 6.25/5 = 1.25, 5 tokens remain
2. Attack crafts a fake RoyaltyVault and calls incrementWindow with royaltyAmount = 5 2-1. The two require checks are easily satisfied since RoyaltyVault can do anything attacker intends it to do 2-2. Balance check is satisfied, since balance(5) >= royaltyAmount(5) 2-3. A new window with royaltyAmount = 5 is added
3. Attacker withdraws his share from the new window 5/5 = 1, 4 tokens remain, attacker withdrew more than he is allowed

In practice, it is possible to repeat the attack several times and siphon the entire royalty pool.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Add a counter to keep track of counted transfers. Function claim and claimForAllWindows should decrease this counter, while incrementWindow should increase this counter. Then upon call to incrementWindow, totalBalance - counter is the actual not yet accounted for royalties. Check this value against the passed royaltyAmount to guard against recounting attacks.

Lines of code

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

Vulnerability details

Impact

Scarcity and uniqueness arguably the core philosophy of NFTs. This bug allows NFT creators(CoreProxy owners) to re-initialize the CoreProxy contract anytime, effectively breaking all promises on scarcity and uniqueness granted upon contract creation. It also allows changing of other fields such as fees for minting, uri for minted NFTs, thus putting buyers at the mercy of contract owner.

Proof of Concept

For proxy contracts, the backing implementation often provides an initialize function to help proxy populate initial state. Since the function is intended for initialization, it is expected to be called just once(just like constructors should only be called once for normal contracts).

However, in the case of CoreCollection.initialize, no modifiers that check whether the contract has already been initialized is used, thus allowing contract owners to re-initialize the contract states whenever they want.

    function initialize(
        string memory _collectionName,
        string memory _collectionSymbol,
        string memory _collectionURI,
        uint256 _maxSupply,
        uint256 _mintFee,
        address _payableToken,
        bool _isForSale,
        address _splitFactory
    ) external onlyOwner onlyValidSupply(_maxSupply) {
        _name = _collectionName;
        _symbol = _collectionSymbol;
        _baseUri = _collectionURI;
        maxSupply = _maxSupply;
        mintFee = _mintFee;
        payableToken = IERC20(_payableToken);
        isForSale = _isForSale;
        splitFactory = _splitFactory;
        initialized = true;
    }

Re-initialize allows re-setting several critical fields, including _baseUri, mineFee and maxSupply, just to name a few. This violates the common guideline of NFTs, and should be mitigated before actual contract deployment.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Add the onlyUnInitialized modifier to initialize.

    function initialize(
        ...
    ) external onlyOwner onlyUninitialized onlyValidSupply(_maxSupply) {
        ...
    }

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L185 https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/ERC721Payable.sol#L50 https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVault.sol#L31

Vulnerability details

Impact

Each CoreProxy is allowed to be associated with a RoyaltyVault, the latter which would be responsible for collecting minting fees and distributing to beneficiaries. Potential mismatch between token used in CoreProxy and RoyaltyVault might result in minting tokens being permanently stuck in RoyaltyVault.

Proof of Concept

Each RoyaltyVault can only handle the royaltyVault.royaltyAsset token assigned upon creation, if any other kind of tokens are sent to the vault, it would get stuck inside the vault forever.

    function sendToSplitter() external override {
        ...
        require(
            IERC20(royaltyAsset).transfer(splitterProxy, splitterShare) == true,
            "Failed to transfer royalty Asset to splitter"
        );
        ...
        require(
            IERC20(royaltyAsset).transfer(
                platformFeeRecipient,
                platformShare
            ) == true,
            "Failed to transfer royalty Asset to platform fee recipient"
        );
        ...
    }

Considering that pairing of CoreProxy and RoyaltyVault is not necessarily handled automatically, and can sometimes be manually assigned, and further combined with the fact that once assigned, CoreProxy does not allow modifications of the pairing RoyaltyVault. We can easily conclude that if a CoreProxy is paired with an incompatible RoyaltyVault, the payableToken minting fees automatically transferred to RoyaltyVault by _handlePayment will get permanently stuck.

     function setRoyaltyVault(address _royaltyVault)
         external
         onlyVaultUninitialized
     {
         ...
         royaltyVault = _royaltyVault;
         ...
     }

     function _handlePayment(uint256 _amount) internal {
         address recipient = royaltyVaultInitialized()
             ? royaltyVault
             : address(this);
         payableToken.transferFrom(msg.sender, recipient, _amount);
         ...
     }

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

While assigning vaults to CoreProxy, check if payableToken is the same as royaltyVault.royaltyAsset

     function setRoyaltyVault(address _royaltyVault)
         external
         onlyVaultUninitialized
     {
         require(
             payableToken == _royaltyVault.royaltyAsset(),
             "CoreCollection : payableToken must be same as royaltyAsset."
         );
         ...
         royaltyVault = _royaltyVault;
         ...
     }

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.

Save gas in batchMint for loops by unchecked arithmetic

The for loop has no overflow risk of i. Use an unchecked block to save gas.

Proof of Concept

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

Recommendation

Use unchecked blocks to avoid overflow checks, or use ++i rather than i++ if you don't use unchecked blocks.

for (uint256 i = 0; i < length; ) {
    ...
    unchecked {
        ++i;
    }
}

Use private constants to save gas

platformFeeRecipient and platformFee can be declared to private constants for gas savings.

Proof of Concept

https://github.com/code-423n4/2022-03-joyn/blob/main/royalty-vault/contracts/RoyaltyVaultFactory.sol#L27-L28

Recommendation

Declare these variables to private.