[H-01] Owner does not get any fee when exercising a put

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L494-L506 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#L443-L457

Vulnerability details

Owner\creators lose their share in the profit by not collecting the fees on half the exercised cases (all puts).

The only place where owner receives fee is when withdrawing an exercised call or expired put: https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L494-L506

But when exercising a put, current code is transferring the whole strike to the exerciser (long).

ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike);

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

Proof of Concept

Consider the following test based on testItWithdrawsAssetsIfPutExercised

    function testItWithdrawsAssetsIfPutExercisedWithFees() public {
        // arrange

        console.log("Setting fees to 2%");
        p.setFee(20);

        PuttyV2.Order memory order = defaultOrder();

        console.log("balance of babe: ", ERC20(order.baseAsset).balanceOf(babe));
        console.log("balance of bob: ", ERC20(order.baseAsset).balanceOf(bob));

        order.isCall = false;
        order.isLong = false;

        console.log("balance of owner at start: ", ERC20(order.baseAsset).balanceOf(p.owner()));

        uint256 erc721TokenId = 88;
        bayc.mint(address(bob), erc721TokenId);
        vm.prank(bob);
        bayc.setApprovalForAll(address(p), true);
        erc721Assets.push(PuttyV2.ERC721Asset({token: address(bayc), tokenId: erc721TokenId}));
        order.erc721Assets = erc721Assets;

        bytes memory signature = signOrder(babePrivateKey, order);
        vm.prank(bob);
        link.approve(address(p), type(uint256).max);
        vm.prank(bob);
        p.fillOrder(order, signature, floorAssetTokenIds);

        console.log("balance of owner after fill: ", ERC20(order.baseAsset).balanceOf(p.owner()));

        PuttyV2.Order memory longOrder = abi.decode(abi.encode(order), (PuttyV2.Order)); // decode/encode to get a copy instead of reference
        longOrder.isLong = true;

        vm.prank(bob);
        p.exercise(longOrder, floorAssetTokenIds);

        console.log("balance of babe: ", ERC20(order.baseAsset).balanceOf(babe));

        console.log("balance should grow by 2% of strike (403001) in compare to when fee is 0%");

        console.log("balance of owner after exercise: ", ERC20(order.baseAsset).balanceOf(p.owner()));

        // act
        vm.prank(babe);
        p.withdraw(order);

        console.log("balance of babe: ", ERC20(order.baseAsset).balanceOf(babe));
        console.log("balance of bob: ", ERC20(order.baseAsset).balanceOf(bob));

        console.log("balance of owner after withdraw: ", ERC20(order.baseAsset).balanceOf(p.owner()));

    }

When using code as it is, setting the fees to any value including 0 result in the same output (In contrast to the fee being collected on Call options)

  balance of babe: , 4294967295
  balance of bob: , 4294967295
  balance of owner at start: , 4294967295
  balance of owner after fill: , 4294967295
  balance of babe: , 4274822143
  balance of owner after exercise: , 4294967295
  balance of babe: , 4274822143
  balance of bob: , 4315112447
  balance of owner after withdraw: , 4294967295

When running the test with 2% fee and my code suggested in mitigation, results show correct handling of the fee:

  Setting fees to 2%
  balance of babe: , 4294967295
  balance of bob: , 4294967295
  balance of owner at start: , 4294967295
  balance of owner after fill: , 4294967295
  balance of babe: , 4274822143
  balance should grow by 2% of strike (403001) in compare to when fee is 0%
  balance of owner after exercise: , 4295370296
  balance of babe: , 4274822143
  balance of bob: , 4314709446
  balance of owner after withdraw: , 4295370296

Tools

forge, editor, code review

Recommended Mitigation Steps

Instead of line L451 mentioned above, add this code:

uint256 feeAmount;
if (fee != 0) {
    feeAmount = (order.strike * fee) / 1000;
    ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
}
ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);

[outdoteth]

Duplicate: Fees are only applied on puts if they are expired: https://github.com/code-423n4/2022-06-putty-findings/issues/269

[HickupHH3]

dup of #285