Ether can be locked in `fillOrder` and `exercise` functions without a way to retrieve it

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L322-L340 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L350-L361 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L422-L443 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L443-L457

Vulnerability details

Impact

A user can choose to pay the option premium and strike with native ETH. In this case, the received ETH is converted to WETH.

However, if a user accidentally sends ETH to the fillOrder or exercise function without having WETH as the order.baseAsset, the sent ETH funds are lost and can not be recovered.

Proof of Concept

fillOrder

There are 2 ways to accidentally lose ETH as a taker:

1. Filling a short position with order.baseAsset != weth, taker accidentally sends ETH to fillOrder

   PuttyV2#L322-L340

   // transfer premium to whoever is short from whomever is long
   if (order.isLong) {
      ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium);
   } else {
      // handle the case where the user uses native ETH instead of WETH to pay the premium
      if (weth == order.baseAsset && msg.value > 0) {
          // check enough ETH was sent to cover the premium
          require(msg.value == order.premium, "Incorrect ETH amount sent");
   
          // convert ETH to WETH and send premium to maker
          // converting to WETH instead of forwarding native ETH to the maker has two benefits;
          // 1) active market makers will mostly be using WETH not native ETH
          // 2) attack surface for re-entrancy is reduced
          IWETH(weth).deposit{value: msg.value}();
          IWETH(weth).transfer(order.maker, msg.value);
      } else { // @audit-info as `order.baseAsset != weth` and msg.value > 0, this else branch is taken and sent ETH is lost
          ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium);
      }
   }

2. Filling a long put position with order.baseAsset != weth, taker accidentally sends ETH to fillOrder

   PuttyV2#L350-L361

   // handle the case where the taker uses native ETH instead of WETH to deposit the strike
   if (weth == order.baseAsset && msg.value > 0) {
      // check enough ETH was sent to cover the strike
      require(msg.value == order.strike, "Incorrect ETH amount sent");
   
      // convert ETH to WETH
      // we convert the strike ETH to WETH so that the logic in exercise() works
      // - because exercise() assumes an ERC20 interface on the base asset.
      IWETH(weth).deposit{value: msg.value}();
   } else { // @audit-info as `order.baseAsset != weth` and msg.value > 0, this else branch is taken and sent ETH is lost
      ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
   }

exercise

There are 2 ways to accidentally lose ETH:

1. Exercising a call option with order.baseAsset != weth, exerciser accidentally sends ETH to exercise

   PuttyV2#L422-L443

   if (order.isCall) {
      // -- exercising a call option
   
      // transfer strike from exerciser to putty
      // handle the case where the taker uses native ETH instead of WETH to pay the strike
      if (weth == order.baseAsset && msg.value > 0) {
          // check enough ETH was sent to cover the strike
          require(msg.value == order.strike, "Incorrect ETH amount sent");
   
          // convert ETH to WETH
          // we convert the strike ETH to WETH so that the logic in withdraw() works
          // - because withdraw() assumes an ERC20 interface on the base asset.
          IWETH(weth).deposit{value: msg.value}();
      } else { // @audit-info as `order.baseAsset != weth` and msg.value > 0, this else branch is taken and sent ETH is lost
          ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
      }
   
      // transfer assets from putty to exerciser
      _transferERC20sOut(order.erc20Assets);
      _transferERC721sOut(order.erc721Assets);
      _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[uint256(orderHash)]);
   }

2. Exercising a put option, exerciser accidentally sends ETH to exercise

   PuttyV2#L443-L457

   } else {
      // -- exercising a put option
      // @audit-info there is no check for msg.value == 0, accidentally sent ETH is lost
      // save the floor asset token ids to the short position
      uint256 shortPositionId = uint256(hashOppositeOrder(order));
      positionFloorAssetTokenIds[shortPositionId] = floorAssetTokenIds;
   
      // transfer strike from putty to exerciser
      ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike);
   
      // transfer assets from exerciser to putty
      _transferERC20sIn(order.erc20Assets, msg.sender);
      _transferERC721sIn(order.erc721Assets, msg.sender);
      _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
   }

Tools Used

Manual review

Recommended mitigation steps

Add checks to enforce msg.value == 0 where appropriate (see @audit-info annotations).

fillOrder

PuttyV2#L322-L364

// transfer premium to whoever is short from whomever is long
if (order.isLong) {
    ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium);
} else {
    // handle the case where the user uses native ETH instead of WETH to pay the premium
    if (weth == order.baseAsset && msg.value > 0) {
        // check enough ETH was sent to cover the premium
        require(msg.value == order.premium, "Incorrect ETH amount sent");

        // convert ETH to WETH and send premium to maker
        // converting to WETH instead of forwarding native ETH to the maker has two benefits;
        // 1) active market makers will mostly be using WETH not native ETH
        // 2) attack surface for re-entrancy is reduced
        IWETH(weth).deposit{value: msg.value}();
        IWETH(weth).transfer(order.maker, msg.value);
    } else {
        require(msg.value == 0, "ETH accidentally sent"); // @audit-info add check

        ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium);
    }
}

// filling short put: transfer strike from maker to contract
if (!order.isLong && !order.isCall) {
    ERC20(order.baseAsset).safeTransferFrom(order.maker, address(this), order.strike);
    return positionId;
}

// filling long put: transfer strike from taker to contract
if (order.isLong && !order.isCall) {
    // handle the case where the taker uses native ETH instead of WETH to deposit the strike
    if (weth == order.baseAsset && msg.value > 0) {
        // check enough ETH was sent to cover the strike
        require(msg.value == order.strike, "Incorrect ETH amount sent");

        // convert ETH to WETH
        // we convert the strike ETH to WETH so that the logic in exercise() works
        // - because exercise() assumes an ERC20 interface on the base asset.
        IWETH(weth).deposit{value: msg.value}();
    } else {
        require(msg.value == 0, "ETH accidentally sent"); // @audit-info add check

        ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
    }

    return positionId;
}

exercise

PuttyV2#L422-L457

if (order.isCall) {
    // -- exercising a call option

    // transfer strike from exerciser to putty
    // handle the case where the taker uses native ETH instead of WETH to pay the strike
    if (weth == order.baseAsset && msg.value > 0) {
        // check enough ETH was sent to cover the strike
        require(msg.value == order.strike, "Incorrect ETH amount sent");

        // convert ETH to WETH
        // we convert the strike ETH to WETH so that the logic in withdraw() works
        // - because withdraw() assumes an ERC20 interface on the base asset.
        IWETH(weth).deposit{value: msg.value}();
    } else {
        require(msg.value == 0, "ETH accidentally sent"); // @audit-info add check

        ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
    }

    // transfer assets from putty to exerciser
    _transferERC20sOut(order.erc20Assets);
    _transferERC721sOut(order.erc721Assets);
    _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[uint256(orderHash)]);
} else {
    // -- exercising a put option

    require(msg.value == 0, "ETH accidentally sent"); // @audit-info add check

    // save the floor asset token ids to the short position
    uint256 shortPositionId = uint256(hashOppositeOrder(order));
    positionFloorAssetTokenIds[shortPositionId] = floorAssetTokenIds;

    // transfer strike from putty to exerciser
    ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike);

    // transfer assets from exerciser to putty
    _transferERC20sIn(order.erc20Assets, msg.sender);
    _transferERC721sIn(order.erc721Assets, msg.sender);
    _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
}

[berndartmueller]

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

[outdoteth]

Duplicate: Native ETH can be lost if it’s not utilised in exercise and fillOrder: https://github.com/code-423n4/2022-06-putty-findings/issues/226