Juicebox contest - cloudjunky's results

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721Delegate.sol#L461 https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721Delegate.sol#L504 https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721Delegate.sol#L635 https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721Delegate.sol#L677

Vulnerability details

Impact

JBTiered721Delegate.sol mints ERC721 tokens after receiving payment. These ERC721 tokens can later be redeemed for a portion of a project’s treasury. However when minting tokens there are no checks to confirm that, if the beneficiary/recipient is a smart contract, it has implemented onERC721Received().

Tokens can be minted to a beneficiary that is a smart contract and those tokens could be trapped there, unable to be redeemed if the smart contract beneficiary has not implemented logic to support the ERC721 standard.

Proof of Concept

JBTiered721Delegate.sol has numerous examples where a functions ultimately call _mint() which is inherited from ERC721.sol. mintFor() is a good example. The function is gated by the onlyOwner() modifier however it still demonstrates the issue;

1. An admin calls the mintFor() function providing the address of a beneficiary that is a smart contract.
2. mintFor() calls _mint(_beneficiary, _tokenId) where the _mint() function is inherited from ERC721.sol.
3. The _mint() function does not perform any checks to ensure that the beneficiary supports the ERC721 standard and transfers ownership of the ERC721 token the beneficiary smart contract.
4. The ownership of the ERC721 token is set to the smart contract beneficiary and cannot be transferred or redeemed.

Tools Used

Vim

Recommended Remediation Steps

Juicebox should use OpenZeppelin’s _safeMint() as opposed to _mint() or perform a check that the recipient of the ERC721 token implements onERC721Received(). Openzeppelin’s _safeMint() is shown below;

function _safeMint(
    address to,
    uint256 tokenId,
    bytes memory data
) internal virtual {
    _mint(to, tokenId);
    require(
        _checkOnERC721Received(address(0), to, tokenId, data),
        "ERC721: transfer to non ERC721Receiver implementer"
    );
}

Replacing references to _mint() with OpenZeppelin’s _safeMint() would ensure that mints are reverted if the beneficiary is a contract and does not support ERC721.