Putty position tokens may be minted to non ERC721 receivers

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L302-L308 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2Nft.sol#L11-L18

Vulnerability details

Putty uses ERC721 safeTransfer and safeTransferFrom throughout the codebase to ensure that ERC721 tokens are not transferred to non ERC721 receivers. However, the initial position mint in fillOrder uses _mint rather than _safeMint and does not check that the receiver accepts ERC721 token transfers:

PuttyV2#fillOrder

        // create long/short position for maker
        _mint(order.maker, uint256(orderHash));

        // create opposite long/short position for taker
        bytes32 oppositeOrderHash = hashOppositeOrder(order);
        positionId = uint256(oppositeOrderHash);
        _mint(msg.sender, positionId);

PuttyV2Nft#_mint

    function _mint(address to, uint256 id) internal override {
        require(to != address(0), "INVALID_RECIPIENT");
        require(_ownerOf[id] == address(0), "ALREADY_MINTED");

        _ownerOf[id] = to;

        emit Transfer(address(0), to, id);
    }

Impact: If a maker or taker are a contract unable to receive ERC721 tokens, their options positions may be locked and nontransferable. If the receiving contract does not provide a mechanism for interacting with Putty, they will be unable to exercise their position or withdraw assets.

Recommendation: Consider implementing the require check in Solmate's ERC721#_safeMint in your own mint function:

    function _safeMint(address to, uint256 id) internal virtual {
        _mint(to, id);

        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, address(0), id, "") ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
    }

However, note that calling _safeMint introduces a reentrancy opportunity! If you make this change, ensure that the mint is treated as an interaction rather than an effect, and consider adding a reentrancy guard:

        /*  ~~~ EFFECTS ~~~ */

        // create opposite long/short position for taker
        bytes32 oppositeOrderHash = hashOppositeOrder(order);
        positionId = uint256(oppositeOrderHash);

        // save floorAssetTokenIds if filling a long call order
        if (order.isLong && order.isCall) {
            positionFloorAssetTokenIds[uint256(orderHash)] = floorAssetTokenIds;
        }

        // save the long position expiration
        positionExpirations[order.isLong ? uint256(orderHash) : positionId] = block.timestamp + order.duration;

        emit FilledOrder(orderHash, floorAssetTokenIds, order);

        /* ~~~ INTERACTIONS ~~~ */

        _safeMint(order.maker, uint256(orderHash));
        _safeMint(msg.sender, positionId);

Alternatively, document the design decision to use _mint and the associated risk for end users.

[outdoteth]

Pasting from another issue:

It's unlikely a contract will have all the setup required to interact with PuttyV2 but not be able to handle ERC721 tokens. Adding a check via safeMint adds a gas overhead as well as another re-entrancy attack vector so there is a tradeoff (as noted in the issue report^^).

[outdoteth]

Report: Contracts that can’t handle ERC721 tokens will lose their Putty ERC721 position tokens

[HickupHH3]

In addition, some contracts may have custom logic in their onERC721Received() implementation that is triggered only by the safe methods and not their "unsafe" counterparts.