search

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

5 stars 0 forks source link

QA Report #219

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Low/QA

No Verification For Unsupported ERC721 Receiver

Permalinks

Description

It is possible that either the maker or the taker is a smart contract that does not support ERC721. This case is possible in 3 out of 4 types, short call is the exception one because the unsupported contract should unable to hold an NFT. Even if it has no significant impact, a user who filled the order with this type of contract may lose opportunities because their underlying will be locked in Putty's contract. Particularly if the user holds a long position, they will have a limited time to exercise the option before it expires, after which their underlying may be locked forever.

Mitigation

There are 2 ways to fix this, the second takes less effort but I'm not recommend:

  1. Decide to support smart contract users. Include the following code at the bottom of PuttyV2Nft._mint() to check that the receiver is a contract that support ERC721 or not. 
    require(
        to.code.length == 0 ||
            ERC721TokenReceiver(to).onERC721Received(msg.sender, address(0), id, "") ==
            ERC721TokenReceiver.onERC721Received.selector,
        "UNSAFE_RECIPIENT"
    );
  2. Decide to not support smart contract users. Implement checking that the receiver should not be a contract, it can be done using OpenZeppelin's Address.isContract(). Please note that the checks may not 100% restrict contracts out of the platform.

Remark: If you decide to support smart contract users, including ERC721TokenReceiver(to).onERC721Received call could lead to reentrancy attack. As it is not possible from the current code base, so I did not focus on finding any.

Any ERC20 Token Can Be Used As baseAsset

Permalinks

Description

Refer to the provided demo site, the baseAsset is implicitly assumed to be ETH/WETH in the most case, so no baseAsset's address is displayed on the front-end. However, creating an order with a custom baseAsset is possible, an attacker could create a fake ERC20 with same name and symbol as the well-known token, USDT, USDC, DAI, for example, and use it as baseAsset. In this case, it is dependent on how the front-end displays it; if this information is displayed imprecisely, users may be deceived. The likely attack in my opinion is long call, in which the attacker uses a fake token as bait to steal ERC20 and/or ERC721 asset(s) from the taker.

Mitigation

Display the baseAsset information clearly on the front end; this should help reduce the risk of users being deceived, but it is still possible. Or else, implement a list of allowed baseAsset so that if any order is placed using a token that is not on the allowed list, the contract will revert, but gas consumption will definitely increase.

DoS with Large Whitelist Array

Permalinks

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

Description

There are no restrictions on the size of the Order.whitelist, which can result in DoS with block gas limit. A user could create an order with an extremely long whitelist address, causing anyone attempting to fill the order to fail and lose their transaction fee.

Mitigation

The array size can be limited by using the reasonable lowest uint size to ensure that no one can include an extremely large array, or by adding a validation check against the Order.whitelist that should not be longer than n length, where n is a reasonable number that will not cause the gas limit to revert while still being sufficient for usage.

Premium Can Be Higher Than Strike

Permalinks

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

Description

Although the premium is expected to be less than the strike amount, there is no safe check. This could result in some attack scenarios, such as when an attacker (maker) creates a short put order with premium = 1 ETH and strike = 0.1 ETH, and when someone takes the attacker order and becomes the taker, the taker pays 1 ETH to the attacker, but can exercise to receive only 0.1 ETH (fee is excluded) back. I noted that such attack scenarios make no sense and are unlikely to occur; however, the contract allows for such an order.

Mitigation

I recommended adding a validation that premium should be less than or equal strike to prevent this kind of order to be happened.

outdoteth commented 2 years ago

No Verification For Unsupported ERC721 Receiver

Duplicate: Contracts that can’t handle ERC721 tokens will lose their Putty ERC721 position tokens: https://github.com/code-423n4/2022-06-putty-findings/issues/327