Lines of code

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

Vulnerability details

Note: This issue only affects the "Short Put" workflow

Vulnerability Details

An order could contain the following:

An invalid ERC20 asset address within the ERC20Asset[] array
An invalid ERC721 NFT address within the ERC721Asset[] array
An invalid floorToken address within the floorTokens[] array

This issue only affects the "Short Put" workflow, where an order maker creates a short put order, and an order taker attempts to fill the short put order. This is because during the "Short Put" workflow:

The malicious order maker (attacker) does not have to transfer any ERC20 asset, ERC721 NFT or floorToken when the order is being filled OR option is being exercised. Thus, this issue will not occur on the attacker-side.
The order taker (victim) does not have to transfer any ERC20 asset, ERC721 NFT or floorToken when filling the malicious order. Thus, the victim will only be aware of this issue when the victim tried to exercise the option, which is too late as the victim has already transferred the premium to the attacker.

Proof-of-Concept

The following illustrates an example of how an attacker could specify an invalid ERC20 Asset within an order to cause the victim to loss funds. The step is the same for an invalid ERC721 NFT or floorToken, thus it is omitted for brevity.

Note: Fee is ignored for simplicity.

Alice (Attacker) create a Short Put order with the following configuration:

Strike Price = 100 ETH

Premium = 5 ETH

isLong = False

isCall = False

baseAsset = WETH

ERC20Asset[] = [0xDAI, 0xUSDC, 0xNONEXIST] // The last address is an invalid erc20 token address that does not exist

Bob (Victim) is not aware that the one of the ERC20 Assets within Alice's order is invalid. Thus, he decided to fill Alice's order by calling PuttyV2.fillOrder. .
Thus, Bob send 5 ETH (premium) to Alice, and Alice transfer 100 ETH (strike price) to PuttyV2 contract for escrow.
At this point, Bob holds a Long Put order, while Alice holds a Short Put order.
At certain point of time, Bob decided to exercise his Long Put option by calling PuttyV2.exercise function. In layman term, Bob wanted to exercise his option to sell the ERC20 assets at the strike price at this point of time.
However, Bob has some issues now. He is able to get the 0xDAI and 0xUSDC ERC20 assets from the open market, but there is no way for Bob to get the 0xNONEXIST ERC20 asset anywhere as it does not exist. Thus, Bob could not exercise his Long Put option because according to the specification/design, he need hold all the 3 ERC20 assets (0xDAI, 0xUSDC, and 0xNONEXIST) in his account to order to exercise his option. Thus, the PuttyV2.exercise function will revert due to missing 0xNONEXIST ERC20 asset in Bob account.
When the Bob's Long Put option expired, Alice proceeds to reclaim back the 100 ETH that she previously escrowed within the PuttyV2 contract.
Alice earns the 5 ETH premium

Alice could initiate multiple malicious orders at a time to 'fish' for victims in Putty Protocol. As long as there is someone willing to fill her order, she will earn the premium in a risk-free environment because the order takers can never exercise their option.

Alice could make the orders more attractive by configuring the order in a manner that will attract potential takers.

Impact

The order taker (victim) would lost their funds as they paid for an option that cannot be exercised.

Recommended Mitigation Steps

It is recommend that the PuttyV2.fillOrder function should perform a sanity check against the "Short Put" order to be filled to ensure the following requirements are in place before filling an order to reduce the risk:

All ERC20 assets specified within the ERC20Asset[] array should point to a valid contract and should be valid (e.g. tokenAmount > 0)
All ERC721 NFTs specified within the ERC721Asset[] array should point to a valid contract and should be valid (e.g. tokenId > 0)
All floorTokens specified within the floorTokens[] array should point to a valid contract

If possible, a whitelisting mechanism should be implemented to only allow approved tokens vetted by Putty to be traded within Putty. The above validation is a best-effort approach to check whether a contract exists at the specified address, but it cannot guarantee that the contract is a valid ERC20 or ERC721 contract, thus it cannot totally eliminate the risk.

The following sanity check could be implemented within the PuttyV2.fillOrder to meet the above-mentioned requirements.

function fillOrder(
    Order memory order,
    bytes calldata signature,
    uint256[] memory floorAssetTokenIds
) public payable returns (uint256 positionId) {
    ..SNIP..
    // filling short put
    if (!order.isLong && !order.isCall) {
            // verify that all ERC20 assets, ERC721 NFTs and floorTokens within an order is valid.
                sanityCheckOnShortPutOrder(order.erc20Assets, order.erc721Assets, order.floorTokens);
    }
    ..SNIP..
}

// verify that all ERC20 assets, ERC721 NFTs and floorTokens within an order is valid.
function sanityCheckOnShortPutOrder(ERC20Asset[] memory assets, ERC721Asset[] memory nfts, address[] memory floorTokens) {
    for (uint256 i = 0; i < assets.length; i++) {
        require(assets[i].token.code.length > 0, "ERC20: Token is not contract");
        require(assets[i].tokenAmount > 0, "ERC20: Amount too small");
    }

    for (uint256 i = 0; i < nfts.length; i++) {
        require(nfts[i].token.code.length > 0, "ERC721: Token is not contract");
        require(nfts[i].tokenId > 0, "ERC721: Invalid token ID");
    }

    for (uint256 i = 0; i < floorTokens.length; i++) {
        require(floorTokens[i].code.length > 0, "FloorToken is not contract");
    }

    // Specify other sanity checks if needed
}

Although client-side or off-chain might have already implemented such validations, simply relying on client-side and off-chain validations are not sufficient. It is possible for an attacker to bypass the client-side and off-chain validations and interact directly with the contract. Thus, such validation must also be implemented in the on-chain contracts.

Additionally, Putty should inform the order taker about the following:

The risks of an invalid ERC20 asset, ERC721 NFT or floorToken within an order
The need for the order taker to perform the necessary due diligence against all the ERC20 assets, ERC721 NFTs and floorTokens within an order before filling it.

code-423n4 / 2022-06-putty-findings