Usage of unsafe `_mint()` function

[code423n4]

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

Proof of Concept

LBTiered721Delegate is using OpenZeppelin's _mint() function. OpenZeppelin discourages the usage of this function. The issue present with _mint() is that it does not check that the receiver accepts ERC721 token transfers.

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

Impact

Impacted functionality for mintReservesFor(), mintFor(), _mintBestAvailableTier() and _mintAll() (contracts that can’t handle ERC721 tokens will lose their JuiceBox ERC721 tokens).

Furthermore, some contract with custom logic on onERC721Received() are only triggered by the safe method.

Recommended Mitigation Steps

Replace _mint() with _safeMint(). Note that _safeMint() adds a reentrancy possibility and therefore functions calling this method should contain a non-rentrancy modifier.

Alternatively, consider documenting the risks of using _mint() and add warnings for for end users.

[drgorillamd]

This is a conscious choice for both gas consumption and reentrancy risk - a contract interacting with a Juicebox terminal might as well receive the project erc20 token, care should be taken when coding such

Indeed, we should document it, thanks

[Picodes]

Downgrading to QA as safeMint is a best practice and is not in the ERC721 strictly speaking

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-10-juicebox-findings/issues/229

[c4-judge]

Picodes marked the issue as change severity