Any failure of a transfer invalidates the whole order, allowing for exercise blocking attacks

[code423n4]

Lines of code

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

Vulnerability details

A malicious user can create an order that can be filled, but cannot be exercised by adding a precooked asset to a bundle that will be transferred to Putty contract, but will reject any outbound transfer, so exercise will not be possible. By adding such an asset to the bundle the attacker guarantees that the taker's premium will be received for free. Such asset will be one from a long list of valuable receivables and can easily go unnoticed.

Setting severity to be high as this opens up a way to use the system for direct stealing. Batch transfers not allowing for any flexibility in general introduce a substantial attack surface as any way to disturb any transfer provides an attack vector.

Proof of Concept

Currently transfers are always required to be completed fully and for the initial list of tokens, even if receiving party would like to omit some assets:

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

            // transfer assets from putty to exerciser
            _transferERC20sOut(order.erc20Assets);
            _transferERC721sOut(order.erc721Assets);
            _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[uint256(orderHash)]);

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

    /**
        @notice Transfers an array of erc20 tokens to the msg.sender.
        @param assets The erc20 tokens and amounts to send.
     */
    function _transferERC20sOut(ERC20Asset[] memory assets) internal {
        for (uint256 i = 0; i < assets.length; i++) {
            ERC20(assets[i].token).safeTransfer(msg.sender, assets[i].tokenAmount);
        }
    }

Recommended Mitigation Steps

Add a way for exerciser to obtain only caller specified subset of order's assets.

I.e. add the asset list argument to exercise and withdraw functions and run the transfers across the list only.

Then the L439-L442 can read, for example:

             // transfer assets from putty to exerciser
-            _transferERC20sOut(order.erc20Assets);
-            _transferERC721sOut(order.erc721Assets);
-            _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[uint256(orderHash)]);
+            _transferERC20sOut(order.erc20Assets, erc20request);
+            _transferERC721sOut(order.erc721Assets, erc721request);
+            _transferFloorsOut(order.floorTokens, floorTokensRequest, positionFloorAssetTokenIds[uint256(orderHash)]);

[GalloDaSballo]

This is equivalent to saying that a user can place an order for a malicious token, yes they can but why?

[dmitriia]

It's not equvalent to a malicious token and exactly this is an issue. Lots of valid tokens revert in some cases and the system have it as an attack surface due to the rigid logic as malicious actors could be able to use this specifics

[csanuragjain]

In this case if malicious user is hiding a precooked asset along with genuine asset then wont the malicious user need to lock in all those genuine assets as well (along with precooked asset) in the fill order call This means malicious user may get the premium but at same time will lose all those genuine assets

Without genuine assets victim wont fill this order. So in this case malicious user may loose more than gaining. Am I missing something?

[dmitriia]

Malicious user will lock genuine assets on fillOrder, but will not lose them in 100% of cases as he just switch the malicious asset state where it revert all the transfers immediately after fillOrder, so exercise will not be possible and attacker's valuable assets are safe.

[csanuragjain]

Ohh gotcha, nice trick

I think I understand the flow now:

1. Assuming short call position, once fillOrder completes all of Attacker valuable assets will be locked in the contract PuttyV2.sol#L368
2. Now Attacker waits for order to expire since user wont be able to exercise
3. After that Attacker withdraws and get the token back

[dmitriia]

Yes, exactly. Assume there is a long list of valuable receivables and an attacker just includes an additional token there. The point is as the token can be blocked there is a big enough probability that taker will just ignore it (ok, I will not receive that one, but I don't need it), not understanding that this will give the attacker the ability to deny the whole exercise.

[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