QA Report

[code423n4]

L01 - Malicious owner() could block withdraw

Transfer of feeAmount in withdraw() function could be reverted by malicious contract on owner address, which would lead to withdrawing block:

            if (fee > 0) {
                feeAmount = (order.strike * fee) / 1000;
                ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
            }

Recommendation: use transfer function for transferring owners fee.

L02 - withdraw() may run out of gas if contracts code of underliying assets would changed in future

Internal functions that withdraw assets _transferERC20sOut, _transferERC721sOut, _transferFloorsOut could only run through all transferring assets in one cycle. If some contracts of transferring assets were updated in future and their transfer would cost more gas, this could lead to inability to withdraw user assets.

Recommendation: Add functionality that allows withdrawing specific assets apart from a batch.

L03 - msg.value could be locked

fillOrder() and exercise() functions should check if msg.value == 0 in case if order.baseAsset != weth. Otherwise user's ether could stuck on contract.

N01 - Missing two-step changing ownership procedure

If the wrong address was used to transfer ownership (for which the private key is unknown) by accident, then it permanently prevents all onlyOwner() functions from being used, including changing various critical parameters.

N02 - Not used import

contracts/src/PuttyV2Nft.sol:5  import "openzeppelin/utils/Strings.sol"; 

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2Nft.sol#L5

N03 - Missing NatSpec

All functions in PuttyV2Nft.sol

[outdoteth]

L03 - msg.value could be locked

Duplicate: Native ETH can be lost if it’s not utilised in exercise and fillOrder: https://github.com/code-423n4/2022-06-putty-findings/issues/226