Multiple instances of reentrancy

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L231 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/ERC721.sol#L193 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L434-L438 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L464 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L112-L116

Vulnerability details

Impact

This can lead to massive loss of funds and asset in Nextgen.

Proof of Concept

There are multiple instances of reentrancy in NextGenCore.sol, MinterContract.sol and AuctionMemo.sol. These reentrancy is due to

• the use of ERC721 _safeMint(...) for token transfer which implements a _checkOnERC721Received(...) and safeTransferFrom return value check on the recipient contract thereby handing transfer over to the recipient contract.

        ....
        require(
            _checkOnERC721Received(address(0), to, tokenId, data),
            "ERC721: transfer to non ERC721Receiver implementer"
        );
        ...

• the use of solidity call() function to transfer value to external recipients thereby handing transfer over tot these contracts. for example in the payArtist(...) function of the MinterContract.sol contract.

        ...
        (bool success1, ) = payable(collectionArtistPrimaryAddresses[colId].primaryAdd1).call{value: artistRoyalties1}("");
        (bool success2, ) = payable(collectionArtistPrimaryAddresses[colId].primaryAdd2).call{value: artistRoyalties2}("");
        (bool success3, ) = payable(collectionArtistPrimaryAddresses[colId].primaryAdd3).call{value: artistRoyalties3}("");
        (bool success4, ) = payable(tm1).call{value: teamRoyalties1}("");
        (bool success5, ) = payable(tm2).call{value: teamRoyalties2}("");
        ...

In both scenarios, a malicious user can perform malicious tasks that could lead to huge loss for the protocol

Tools Used

Manual

Recommended Mitigation Steps

Implement a reentrancy guard in all state altering and state reading functions in the NextGenCore.sol, MinterContract.sol and AuctionMemo.sol contracts respectively

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as duplicate of #2039

[c4-pre-sort]

141345 marked the issue as duplicate of #51

[c4-pre-sort]

141345 marked the issue as duplicate of #1742

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

The Warden has marked a few instances where re-entrancy could occur and some of them do indeed lead to fund loss, such as in the AuctionDemo contract and #1547.

However, the Warden has simply marked these instances as a static analyzer would and has failed to elaborate on the why and how. As such, I consider this exhibit to have insufficient proof.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Insufficient proof