Joyn contest - wuwe1's results

Lines of code

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

Vulnerability details

Proof of Concept

1. incrementWindow can be called by anyone multiple times.

https://github.com/code-423n4/2022-03-joyn/blob/main/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;
    }

2. Assume alice deposit 10e18 weth in the Splitter and call increateWindow. Now currentWindow is 1. Bob get 10 percent of alice's deposit. Bob call increateWindow with royaltyAmount == 10e18 9 times. Now currentWindow is 10. Finally, bob call claimForAllWindows and get 10e18 weth.

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

    function claimForAllWindows(
        uint256 percentageAllocation,
        bytes32[] calldata merkleProof
    ) external {
        // Make sure that the user has this allocation granted.
        require(
            verifyProof(
                merkleProof,
                merkleRoot,
                getNode(msg.sender, percentageAllocation)
            ),
            "Invalid proof"
        );

        uint256 amount = 0;
        for (uint256 i = 0; i < currentWindow; i++) {
            if (!isClaimed(msg.sender, i)) {
                setClaimed(msg.sender, i);

                amount += scaleAmountByPercentage(
                    balanceForWindow[i],
                    percentageAllocation
                );
            }
        }

        transferSplitAsset(msg.sender, amount);
    }

Recommended Mitigation Steps

If the balance is not change, the currentWindow is not suppposed to change.

Consider introduce a variable tracking token deposit in current window and reset this variable in new window.

Lines of code

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#L236

Vulnerability details

Proof of Concept

A simple POC: https://gist.github.com/wuwe1/9eb5bf9e4b3f31c8db52f4fa7fac5b13

royalty-vault/contracts/RoyaltyVault.sol
44:            IERC20(royaltyAsset).transfer(splitterProxy, splitterShare) == true,
52:            IERC20(royaltyAsset).transfer(

splits/contracts/Splitter.sol
236:        didSucceed = IERC20(splitAsset).transfer(to, value);

Fund sent directly to these contract will locked due to this incompatibility.

Recommended Mitigation Steps

Consider using SafeERC20

Lines of code

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

Vulnerability details

Impact

This is dangerous in scam senario because the malicious user can frontrun and become the owner of the collection. As owner, one can withdraw paymentToken. (note that _collections.isForSale can be change by frontrunner)

Proof of Concept

1. Anyone can call createProject.

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

  function createProject(
    string memory _projectId,
    Collection[] memory _collections
  ) external onlyAvailableProject(_projectId) {
    require(
      _collections.length > 0,
      'CoreFactory: should have more at least one collection'
    );

Recommended Mitigation Steps

Two way to mitigate.

1. Consider use white list on project creation.
2. Ask user to sign their address and check the signature against msg.sender. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L102

Lines of code

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

Vulnerability details

Proof of Concept

        require(
            IRoyaltyVault(msg.sender).getSplitter() == address(this),
            "Unauthorised to increment window"
        );

The authorization check in incrementWindow can be easily passed. However, incrementWindow can change the value of currentWindow which is used for loop. It should be guard properly.

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

for (uint256 i = 0; i < currentWindow; i++) {

Recommended Mitigation Steps

Consider using white list for this task.