Use safeMint for ERC721

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L206

Vulnerability details

🎨 Category

ERC721, Data Validation

💥 Impact

In Auction.sol, the _createAuction() function eventually calls mint() in Token.sol.

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L143

Calling mint this way does not ensure that the settings.auction can handle ERC721 properly.

📝 Proof of Concept

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L206

🚜 Tools Used

Manual

✅ Recommended Mitigation Steps

Use _safeMint() function of Opnezeppelin instead of the mint function.
And this function should be used with reentrancy guards as a guard to protect the user.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol

function _safeMint(address to, uint256 tokenId) internal virtual {
     _safeMint(to, tokenId, "");
 }
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"
    );
}

👬 Similar Issue

code-423n4/2022-01-sandclock-findings#29

[GalloDaSballo]

I believe that adding this check can be used for DOS and probably will cause more issues than it solves.

I also would find it very odd for a caller to bid on an NFT they can't move

[GalloDaSballo]

After further consideration, I believe this design decision was made on purpose, any msg.sender that decides to interact with the protocol is expected to be able to receive the tokens they are bidding for.

Not being able to do that is equivalent to them transferring the tokens to the address(0) (or delegating to 0, per other reports from this contest)

Meaning is completely within the decision of the bidder.

Allowing to revert on mint creates more problems than necessary, adds further complexity, increase gas cost and increases the surface area.

Because I believe the Sponsor has gone through these ideas, I belive the finding to be invalid

I'd recommend the sponsor to warn users about being able to move tokens they are buying for, however at this time am choosing those close these reports as invalid as I don't think the implementation is expected, and the reference example of Nouns Dao has been working this way for over a year