Attacker can steal other users funds by signing malicious orders which looks profitable and tricking users (or by mistake) to call fillOrder(), the malicious order transfers PuttyV2 NFT token position of caller to attacker

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2Nft.sol#L20-L37 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L259-L380

Vulnerability details

Impact

by calling fillOrder() contract mint two NFT token position for Long and Short position for maker and taker and also code transfers premium (which is based on order.baseAsset) from Long position owner to Short position owner and transfers collaterals from Short position owner to PuttyV2 address so Long positions owner can claim those collaterals by calling exercise() and paying the strike price. all the address of tokens are defined in order object so Attacker can sign malicious order which look profitable but transfers NFT position token of caller to attacker (set NFT position token of caller as premium or collateral, the ID of caller NFT position token is calculable when attacker signs the order) and if any user tries to fill that order he will lose his funds. so the impact of this bug is that users would lose their funds by filling orders.

Proof of Concept

To exploit this attacker have multiple choices, the real bug is that contract allows token address in order to be equal to PuttyV2 address, it means attacker can create and sign an order which look very good for filler but when someone calls fillOrder() for that order, code would mint caller an NFT position token but transfer that NFT to attacker as premium or to contract as collateral(which attacker can claim later) and user would lose his funds. the detail steps of exploit is:

1. attacker would sign this order (order.isLong=False & order.baseAsset=PuttyV2 & order.premium=oppositeOrderHash & )
2. attacker would give all token spending allowance to PuttyV2 address.
3. user would see attacker order and as it is looks very profitable he would call fillOrder() for that order. (attacker can somehow trick users to click on fillOrder() too by creating a lot of order)
4. fillOrder logic would mint NFT positions for caller but in the line 338 (ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium)) code would transfer NFT position token of caller to the attacker address because: (order.baseAsset == PuttyV2 and msg.sender == user and order.maker == attacker and order.premium == caller NFT position token ID) and also code would transfer other user tokens that are specified in order as collateral.
5. users funds would be transferred into contract but user didn't receive any NFT position token and the NFT position token of Long and Short position would be in attacker address. (attacker can create PUT option and transfer user funds as collateral) and attacker would receive both Long and Short NFT position token.

The other thing that attacker do is in this steps:

1. attacker would sign this order: (order.isLong=True & order.isCall=True & order.erc721Assets=(address=PuttyV2, id=oppositeOrderHash) & )
2. attacker would give all token spending allowance to PuttyV2 address.
3. user would see attacker order and as it is looks very profitable he would call fillOrder() for that order. (attacker can somehow trick users to click on fillOrder() too by creating a lot of order)
4. fillOrder logic would mint NFT positions for caller but in the line 376 (_transferERC721sIn(order.erc721Assets, msg.sender);) code would transfer NFT position token of caller to the PuttyV2 address because: (order.erc721Assets == [(address=PuttyV2, id= caller NFT token ID)] and msg.sender == user) and also code would transfer other user tokens that are specified in order as collateral.
5. users funds would be transferred into contract as collateral but user didn't receive any NFT position token and the NFT position token of Long and Short position would be in attacker address. attacker would call exercise() and withdraw() to get his and user tokens from contract.

So because contract logics allows token addresses in order to be PuttyV2 address it's possible to create malicious order that it seems very profitable for caller but in fact it transfers user NFT position token to contract as collateral or to attacker as premium so user couldn't use the filled order position after calling fillOrder() and his funds would be lost.

Tools Used

VIM

Recommended Mitigation Steps

don't allow the token address in order to be PuttyV2 address.

[GalloDaSballo]

The warden is saying that:

• A user would accept a malicious order of their volition
• The transferFrom with PuttyV2 would not revert

I believe both are questionable assumptions

[outdoteth]

Duplicate: Setting malicious or invalid erc721Assets, erc20Assets or floorTokens prevents the option from being exercised: https://github.com/code-423n4/2022-06-putty-findings/issues/50