Fake orders can be created for non existant tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L43

Vulnerability details

Impact

NFT's can be stolen by an attacker

Proof of Concept

The putty contract is using the solmate safeTransfer instead of OpenZeppelin's so contract existence isn't being checked for transferred tokens. https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L43

In the putty case, contract existance is checked for the base asset https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L293 But not for the NFTs or other ERC20s included in the order. https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L80 OZ library should be used to check the existence of all tokens in the arrays on the order.

An attacker could make an order with a real base asset, premium, and strike of 0 with an NFT that wasn't still deployed. He can then predict a future address of some project that deploys NFT's in a factory since they are deterministic and make an order with that nonexistent NFT. If the NFT is then deposited into putty the option can be exercised to get the NFT.

The issue is equal to the following issue on the cally contest submitted by WatchPug and other wardens (due credits to them): https://github.com/code-423n4/2022-05-cally-findings/issues/225

Recommended Mitigation Steps

Use OZ library or check contract existance for all assets in an order and not just base asset

[kirk-baird]

Code size is checked on transfers in for ERC20s https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L598

[Pedroais]

Code size is checked on transfers in for ERC20s https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L598

Hey, I actually meant the ERC721's . I became aware of the mistake after submitting it and sent a help request to edit the submission but it wasn't responded since the team is on holiday. Your comment is true but please wait until the edit is made by the team before judging the submission.

[ghost]

Code size is checked on transfers in for ERC20s https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L598

Hey, I actually meant the ERC721's . I became aware of the mistake after submitting it and sent a help request to edit the submission but it wasn't responded since the team is on holiday. Your comment is true but please wait until the edit is made by the team before judging the submission.

ERC721.safeTransferFrom uses solmate's ERC721 impl which checks codesize. https://github.com/Rari-Capital/solmate/blob/main/src/tokens/ERC721.sol#L111-L124

[Pedroais]

Code size is checked on transfers in for ERC20s https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L598

Hey, I actually meant the ERC721's . I became aware of the mistake after submitting it and sent a help request to edit the submission but it wasn't responded since the team is on holiday. Your comment is true but please wait until the edit is made by the team before judging the submission.

ERC721.safeTransferFrom uses solmate's ERC721 impl which checks codesize. https://github.com/Rari-Capital/solmate/blob/main/src/tokens/ERC721.sol#L111-L124

Then you are right on this one :)

[outdoteth]

As @STYJ said, this attack does not make sense because the codesize is checked in solmate's ERC721 safeTransferFrom already.