Joyn contest - WatchPug's results

Lines of code

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

Vulnerability details

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

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;
}

PoC

Given:

• Splitter got 10,000 USDC in balance
• Alice owns 10%
• currentWindow = 1

1. Alice creates a malicous contract which includes methods of supportsInterface and getSplitter;
2. Alice call incrementWindow() 10 times with the malicious contract with royaltyAmount = 10,000 USDC;
3. Alice claim window 1~10 got 1,000 USDC each, 10,000 USDC in total.

Recommendation

Consider changing to:

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

    require(
        IRoyaltyVault(msg.sender).supportsInterface(IID_IROYALTY),
        "Royalty Vault not supported"
    );

    IERC20(royaltyAsset).transferFrom(msg.sender, address(this), royaltyAmount);
    balanceForWindow.push(royaltyAmount);
    currentWindow += 1;
    emit WindowIncremented(currentWindow, royaltyAmount);
    return true;
}

Lines of code

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

Vulnerability details

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

    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;
    }

initialize() should only be callable when initialize != true.

Otherwise, the Owner can call it again and change all sorts of critical settings of the contract, and cause severe impact on prior users.

PoC

1. A NFT with a 100 of maxSupply and 1 ETH of mint fee is initialized;
2. All soldout and the floor price is now 10 ETH on OpenSea;
3. A malicious/compromised owner can call initialize() and set _maxSupply to 99999 and _mintFee to 1 FAKETOKEN, which is issued by the attacker himself, therefore, no other can mint.
4. The owner mint and dump on OpenSea, causing a huge price impact and making all the prior minters and buyers of the NFT lose money.

Recommendation

Add onlyUnInitialized to initialize():

    function initialize(
        string memory _collectionName,
        string memory _collectionSymbol,
        string memory _collectionURI,
        uint256 _maxSupply,
        uint256 _mintFee,
        address _payableToken,
        bool _isForSale,
        address _splitFactory
    ) external onlyOwner onlyUnInitialized onlyValidSupply(_maxSupply) {

Lines of code

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

Vulnerability details

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

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

There is no upper bound of _platformFee.

Impact

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

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

If the malicious/compromised owner sets _platformFee to a number of 10000.

When sendToSplitter(), the attacker will get all the funds.

Recommendation

Consider introducing a reasonable upper bound for the _platformFee.

    function setPlatformFee(uint256 _platformFee) external override onlyOwner {
        require(_platformFee <= 5000, "invalid platformFee") // max 50%
        platformFee = _platformFee;
        emit NewRoyaltyVaultPlatformFee(_platformFee);
    }

Lines of code

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

Vulnerability details

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

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

Unlike ERC721, most ERC20 tokens require an allowance when calling transferFrom(), even when the from == msg.sedner.

Therefore, in the withdraw() function, it should be using payableToken.trasnfer(msg.sender, amount); instead of payableToken.transferFrom().

The current implementation will most certainly revert with the error: "ERC20: insufficient allowance" for almost all ERC20 tokens.

See: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L164

Recommendation

Change to: payableToken.trasnfer(msg.sender, amount);

Lack of access control allows anyone to steal all most all the funds from the vaults created by RoyaltyVaultFactory

Lines of code

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

Vulnerability details

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

    function setPlatformFeeRecipient(
        address _vault,
        address _platformFeeRecipient
    ) external {
        require(_vault != address(0), "Invalid vault");
        require(
            _platformFeeRecipient != address(0),
            "Invalid platform fee recipient"
        );
        IRoyaltyVault(_vault).setPlatformFeeRecipient(_platformFeeRecipient);
    }
}

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

    function setPlatformFee(address _vault, uint256 _platformFee) external {
        IRoyaltyVault(_vault).setPlatformFee(_platformFee);
    }

PoC

The attacker (any address) can call setPlatformFee and change the PlatformFeeRate to 100% and then setPlatformFeeRecipient to their address and just call sendToSplitter() on the vault and steal all the funds.

Recommendation

Change to:

    function setPlatformFeeRecipient(
        address _vault,
        address _platformFeeRecipient
    ) external onlyOwner{
        require(_vault != address(0), "Invalid vault");
        require(
            _platformFeeRecipient != address(0),
            "Invalid platform fee recipient"
        );
        IRoyaltyVault(_vault).setPlatformFeeRecipient(_platformFeeRecipient);
    }
}

    function setPlatformFee(address _vault, uint256 _platformFee) external onlyOwner{
        IRoyaltyVault(_vault).setPlatformFee(_platformFee);
    }

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[S] Avoid unnecessary external call can save gas

Every call to an external contract costs a decent amount of gas. For optimization of gas usage, avoiding unnecessary external call can save gas if possible.

For example:

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

    function createVault(address _splitter, address _royaltyAsset)
        external
        returns (address vault)
    {
        splitterProxy = _splitter;
        royaltyAsset = _royaltyAsset;

        vault = address(
            new ProxyVault{salt: keccak256(abi.encode(_splitter))}()
        );

        delete splitterProxy;
        delete royaltyAsset;
    }

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

    constructor() {
        royaltyVault = IVaultFactory(msg.sender).royaltyVault();
        splitterProxy = IVaultFactory(msg.sender).splitterProxy();
        royaltyAsset = IVaultFactory(msg.sender).royaltyAsset();
        platformFee = IVaultFactory(msg.sender).platformFee();
        platformFeeRecipient = IVaultFactory(msg.sender).platformFeeRecipient();
    }

Can be changed to:

    constructor(address _royaltyVault, address _splitterProxy, address _royaltyAsset, uint256 _platformFee, address _platformFeeRecipient) {
        royaltyVault = _royaltyVault;
        splitterProxy = _splitterProxy;
        royaltyAsset = _royaltyAsset;
        platformFee = _platformFee;
        platformFeeRecipient = _platformFeeRecipient;
    }

    function createVault(address _splitter, address _royaltyAsset)
        external
        returns (address vault)
    {
        splitterProxy = _splitter;
        royaltyAsset = _royaltyAsset;

        vault = address(
            new ProxyVault{salt: keccak256(abi.encode(_splitter))}(royaltyVault, splitterProxy, royaltyAsset, platformFee, platformFeeRecipient)
        );

        delete splitterProxy;
        delete royaltyAsset;
    }

It can save 5 times of external calls.

[M] Setting uint256 variables to 0 is redundant

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

        uint256 amount = 0;

Setting uint256 variables to 0 is redundant as they default to 0.

Other examples include:

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

[S] Cache array length in for loops can save gas

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Instances include:

• CoreFactory.sol#createProject()

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

• Splitter.sol#verifyProof()

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

[M] ++i is more efficient than i++

Using ++i is more gas efficient than i++, especially in for loops.

For example:

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

        for (uint256 i = 0; i < proof.length; i++)

Change to:

        for (uint256 i = 0; i < proof.length; +i)

[M] Unused constant variable

Unused constant variables in contracts increase contract size and gas usage at deployment.

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

    uint256 public constant PERCENTAGE_SCALE = 10e5;

In the contract, the constant variable PERCENTAGE_SCALE is set once but has never been read, therefore it can be removed.

[M] Use short reason strings can save gas

Every reason string takes at least 32 bytes.

Use short reason strings that fits in 32 bytes or it will become more expensive.

Instances include:

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

require(amount > 0, "CoreCollection: Amount should be greater than 0");

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

        require(
            msg.sender == splitFactory || msg.sender == owner(),
            "CoreCollection: Only Split Factory or owner can initialize vault."
        );

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

        require(
            startingIndex == 0,
            "CoreCollection: Starting index is already set"
        );

[M] Unused events

Unused events increase contract size and gas usage at deployment.

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

    event VaultCreated(address vault);

VaultCreated is unused.

[M] Unused function parameters

Unused private function increase contract size and gas usage at deployment.

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

    function attemptETHTransfer(address to, uint256 value)
        private
        returns (bool)
    {
        // Here increase the gas limit a reasonable amount above the default, and try
        // to send ETH to the recipient.
        // NOTE: This might allow the recipient to attempt a limited reentrancy attack.
        (bool success, ) = to.call{value: value, gas: 30000}("");
        return success;
    }

attemptETHTransfer() is unused.

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

    function amountFromPercent(uint256 amount, uint32 percent)
        private
        pure
        returns (uint256)
    {
        // Solidity 0.8.0 lets us do this without SafeMath.
        return (amount * percent) / 100;
    }

amountFromPercent() is unused.

[S] Use immutable instead of getter to save gas

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

contract CoreProxy is Ownable {
    address private immutable _implement;

    constructor(address _imp) {
        _implement = _imp;
    }

    fallback() external {
        address _impl = implement();
        assembly {
            let ptr := mload(0x40)
            calldatacopy(ptr, 0, calldatasize())
            let result := delegatecall(gas(), _impl, ptr, calldatasize(), 0, 0)
            let size := returndatasize()
            returndatacopy(ptr, 0, size)

            switch result
            case 0 {
                revert(ptr, size)
            }
            default {
                return(ptr, size)
            }
        }
    }

    function implement() public view returns (address) {
        return _implement;
    }
}

Recommendation

Change to:

contract CoreProxy is Ownable {
    address private immutable _implement;

    constructor(address _imp) {
        _implement = _imp;
    }

    fallback() external {
        address _impl = _implement;
        assembly {
            let ptr := mload(0x40)
            calldatacopy(ptr, 0, calldatasize())
            let result := delegatecall(gas(), _impl, ptr, calldatasize(), 0, 0)
            let size := returndatasize()
            returndatacopy(ptr, 0, size)

            switch result
            case 0 {
                revert(ptr, size)
            }
            default {
                return(ptr, size)
            }
        }
    }

    function implement() external view returns (address) {
        return _implement;
    }
}