Short Call Maker Asset Can Be Frozen

[code423n4]

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, ...);

[ghost]

Duplicate of https://github.com/code-423n4/2022-06-putty-findings/issues/369

[outdoteth]

Short call with floorTokens will result in a revert when exercising: https://github.com/code-423n4/2022-06-putty-findings/issues/369