Putty contest - Kenshin's results

Lines of code

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

Vulnerability details

Impact

The fillOrder() function only checks that the floorAssetTokenIds.length must be 0 when a taker fill a short call order. In other words, it does not check whether order.floorTokens.length is 0 or not, which means that if the maker includes any address in order.floorTokens, the maker will be unable to withdraw their assets when the order expires. The function withdraw() and exercise() will run into uninitialize variable, which causes the Index out of bound revert due to length inconsistency between two arrays.

Proof of Concept

1. Babe (whose asset will be frozen) create and sign a short call order.
2. Someone fill Babe's order, Babe's assets will be transferred to Putty.
   • If the taker attempt to exercise the order, he/she will encounter index out of bounds revert as well.
3. The order has not been exercised, and become expire.
4. Babe attempt to withdraw but always encounter index out of bounds revert. Babe's assets will be frozen in Putty.

Run the test with Foundry.

function testShortCallNonWithdrawable() public {
        PuttyV2.Order memory babeOrder = defaultOrder();
        
        // short call order
        babeOrder.isLong = false;
        babeOrder.isCall = true;
        
        // add BAYC #1 as an asset
        erc721Assets.push(PuttyV2.ERC721Asset({token: address(bayc), tokenId: 1}));
        babeOrder.erc721Assets = erc721Assets;
        
        // include BAYC address to floorTokens array
        floorTokens.push(address(bayc));
        babeOrder.floorTokens = floorTokens;
        
        //Babe sign order
        bytes memory signature = signOrder(babePrivateKey, babeOrder);

        // Mint BAYC #1 to Babe and Approve to Putty
        bayc.mint(babe, 1);
        vm.prank(babe);
        bayc.approve(address(p), 1);

        //Someone fill Babe order, floorAssetTokenIds must be empty
        p.fillOrder(babeOrder, signature, floorAssetTokenIds);
        
        assertEq(bayc.ownerOf(1), address(p), "Babe's BAYC #1 should be transferred to Putty");

        /* Exercise revert */
        babeOrder.isLong = !babeOrder.isLong;
        vm.expectRevert(stdError.indexOOBError);
        p.exercise(babeOrder, floorAssetTokenIds);
        
        /* Withdraw revert */
        //Let it expires
        vm.warp(block.timestamp + babeOrder.duration + 1 days);
        babeOrder.isLong = !babeOrder.isLong;

        // It should revert with "Index out of bounds"
        vm.expectRevert(stdError.indexOOBError);
        vm.prank(babe);
        p.withdraw(babeOrder);
        
        assertEq(bayc.ownerOf(1), address(p), "BAYC #1 should be stuck in Putty");

Tools Used

Foundry

Recommended Mitigation Steps

Add an empty array validation to invoke floorToken transfer only when positionFloorAssetTokenIds[floorPositionId] is not empty.

if (positionFloorAssetTokenIds[floorPositionId].length > 0) _transferFloorsOut(order.floorTokens, ...);

Judge has assessed an item in Issue #219 as Medium risk. The relevant finding follows:

Low/QA

No Verification For Unsupported ERC721 Receiver

Permalinks

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

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

• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L324
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L338
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L344
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L360
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L436
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L451
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L500
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L503

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.

Gas Optimization

Check Asset Array Not Empty Before Transfer Can Save More Gas

Permalinks

• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L368-L369
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L368-L377
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L440-L442
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L454-L456

Description

Transferring assets in and out of Putty always be executed even the asset array is empty. Checking and skip transferring call over empty asset array can help save some gas.

Mitigation

Add a check against asset array and skip the transferring if it is empty. Here is some example:

//PuttyV2.sol:L375-L377
if (order.erc20Assets.length > 0) _transferERC20sIn(order.erc20Assets, msg.sender);
if (order.erc721Assets.length > 0) _transferERC721sIn(order.erc721Assets, msg.sender);
if (order.floorTokens.length > 0) _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);

Prefix Incremental and Unchecked Block in For-Loop Can Save More Gas

Permalinks

• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L556
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L594
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L611
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L627
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L637
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L647
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L658
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L670
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L728
• https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L742

Description

Prefix incremental ++i uses slightly less gas than postfix incremental i++. And on arithmetic operation that can be guarantee to not overflow/underflow, the unchecked block should be apply for gas optimization opportunity.

Mitigation

Use prefix instead of postfix incremental, and apply the unchecked block over ++i. Here is an example:

//PuttyV2.sol:L610-L614
function _transferERC721sIn(ERC721Asset[] memory assets, address from) internal {
        for (uint256 i = 0; i < assets.length; ) {
            ERC721(assets[i].token).safeTransferFrom(from, address(this), assets[i].tokenId);
            
            unchecked {
                ++i;
            }
        }
    }