`fillOrder` allow the order to have `order.premium == 0`

[code423n4]

Lines of code

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

Vulnerability details

Impact

The premium property of an order define the amount of ETH/ERC20 (it depends on order.baseAsset) that the "longer" user has to send to the "shorter" user.

Currently, the contract allow having orders with premium == 0. This mean that the "shorter" will not be paid to provide the option.

Without a premium, the "longer" could use interact with the contract without having anything to lose (they just need to wait for the option to expire).

Proof of Concept

• Alice creates and signs a long put option order off-chain for 2 Bored Ape floors with a duration of 30 days, a strike of 124 WETH and a premium of 0 WETH
• Bob takes Alice's order and fills it by submitting it to the Putty smart contract using fillOrder()
• Bob sends 124 ETH to cover the strike, which is converted to WETH. 0 WETH is transferred from Alice's wallet to Bob's wallet.
• 17 days pass and the floor price for Bored Apes has dropped to 54 ETH
• Alice exercise her long put, follow all the steps (buy NFT, send them to the contract and so on) and make a total profit of 16 ETH
• Bob withdraw the Bored Apes (having "paid them" more than the value of them 17 days ago) and get 0 PREMIUM

Tools Used

Manual review + forge test. Here is the test to showcase this issue

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "forge-std/console.sol";
import "openzeppelin/utils/cryptography/ECDSA.sol";

import "src/PuttyV2.sol";
import "./shared/Fixture.t.sol";

contract TestC4ZeroPremium is Fixture {
    event WithdrawOrder(bytes32 indexed orderHash, PuttyV2.Order order);

    address[] internal floorTokens;
    PuttyV2.ERC20Asset[] internal erc20Assets;
    PuttyV2.ERC721Asset[] internal erc721Assets;
    uint256[] internal floorAssetTokenIds;

    receive() external payable {}

    function setUp() public {
        deal(address(weth), address(this), 0xffffffff);
        deal(address(weth), babe, 0xffffffff);

        weth.approve(address(p), type(uint256).max);

        vm.prank(babe);
        weth.approve(address(p), type(uint256).max);
    }

    function testItSendsZeroPremiumWETHToMakerIfShort() public {
        // arrange
        PuttyV2.Order memory order = defaultOrder();
        order.premium = 0;
        order.isLong = false;
        bytes memory signature = signOrder(babePrivateKey, order);

        uint256 takerBalanceBefore = weth.balanceOf(address(this));
        uint256 makerBalanceBefore = weth.balanceOf(order.maker);

        // act
        p.fillOrder(order, signature, floorAssetTokenIds);

        // assert
        assertEq(weth.balanceOf(order.maker) - makerBalanceBefore, 0, "Should have transferred premium to maker");

        assertEq(takerBalanceBefore - weth.balanceOf(address(this)), 0, "Should have transferred premium from taker");
    }

    function testItSendsZeroPremiumERC20ToMakerIfShort() public {
        // arrange
        PuttyV2.Order memory order = defaultOrder();
        order.baseAsset = address(link);
        order.premium = 0;
        order.isLong = false;
        bytes memory signature = signOrder(babePrivateKey, order);

        uint256 takerBalanceBefore = link.balanceOf(address(this));
        uint256 makerBalanceBefore = link.balanceOf(order.maker);

        // act
        p.fillOrder(order, signature, floorAssetTokenIds);

        // assert
        assertEq(link.balanceOf(order.maker) - makerBalanceBefore, 0, "Should have transferred premium to maker");

        assertEq(takerBalanceBefore - link.balanceOf(address(this)), 0, "Should have transferred premium from taker");
    }

    function testItSendsZeroPremiumWETHToTakerIfLong() public {
        // arrange
        PuttyV2.Order memory order = defaultOrder();
        order.premium = 0;
        order.isLong = true;
        bytes memory signature = signOrder(babePrivateKey, order);

        uint256 takerBalanceBefore = weth.balanceOf(address(this));
        uint256 makerBalanceBefore = weth.balanceOf(order.maker);

        // act
        p.fillOrder(order, signature, floorAssetTokenIds);

        // assert
        assertEq(weth.balanceOf(address(this)) - takerBalanceBefore, 0, "Should have transferred premium to taker");

        assertEq(makerBalanceBefore - weth.balanceOf(order.maker), 0, "Should have transferred premium from maker");
    }

    function testItSendsZeroPremiumERC20ToTakerIfLong() public {
        // arrange
        PuttyV2.Order memory order = defaultOrder();
        order.baseAsset = address(link);
        order.premium = 0;
        order.isLong = true;
        bytes memory signature = signOrder(babePrivateKey, order);

        uint256 takerBalanceBefore = link.balanceOf(address(this));
        uint256 makerBalanceBefore = link.balanceOf(order.maker);

        // act
        p.fillOrder(order, signature, floorAssetTokenIds);

        // assert
        assertEq(link.balanceOf(address(this)) - takerBalanceBefore, 0, "Should have transferred premium to taker");

        assertEq(makerBalanceBefore - link.balanceOf(order.maker), 0, "Should have transferred premium from maker");
    }
}

Recommended Mitigation Steps

Prevent the user to create off-chain orders with premium == 0.

Consider adding a check on fillOder that will revert if order.premium == 0.

[outdoteth]

Duplicate: Possible to create spam orders with 0 strike and premium: https://github.com/code-423n4/2022-06-putty-findings/issues/305

[HickupHH3]

Part of warden's QA: #199