Fix vulnerability that allows bidders to block people who will outbid

src/HookCoveredCallImplV1.sol

@@ -18,6 +18,7 @@ import "./interfaces/IHookERC721VaultFactory.sol";
 import "./interfaces/IHookERC721Vault.sol";
 import "./interfaces/IHookCoveredCall.sol";
 import "./interfaces/IHookProtocol.sol";
+import "./interfaces/IWETH.sol";
 
 /// @title HookCoveredCallImplV1 an implementation of covered calls on Hook
 /// @author Jake Nyquist -- [email protected]
@@ -74,6 +75,9 @@ contract HookCoveredCallImplV1 is
   /// insturment.
   address public allowedNftContract;
 
+  /// @dev the address of WETH on the chain where this contract is deployed
+  address public weth;
+
   /// --- Constructor
   // the constructor cannot have arugments in proxied contracts.
   constructor() ERC721("CallOption", "CALL") {}
@@ -91,6 +95,7 @@ contract HookCoveredCallImplV1 is
   ) public initializer {
     _protocol = IHookProtocol(protocol);
     _erc721VaultFactory = IHookERC721VaultFactory(hookVaultFactory);
+    weth = _protocol.getWETHAddress();
     allowedNftContract = _allowedNftContract;
   }
 
@@ -264,8 +269,7 @@ contract HookCoveredCallImplV1 is
     require(bidAmt > call.strike, "bid - bid is lower than the strike price");
 
     // return current bidder's money
-    (bool sent, ) = call.highBidder.call{value: call.bid}("");
-    require(sent, "Failed to send Ether");
+    _safeTransferETHWithFallback(call.highBidder, call.bid);
 
     // set the new bidder
     call.bid = bidAmt;
@@ -335,15 +339,10 @@ contract HookCoveredCallImplV1 is
     uint256 spread = call.bid - call.strike;
 
     // return send option holder their earnings
-    (bool sent, ) = ownerOf(optionId).call{value: spread}("");
-    require(sent, "Failed to send Ether to option holder");
-
-    // return send option holder their earnings
-    (
-      bool writerSent, /* bytes memory writerData */
+    _safeTransferETHWithFallback(ownerOf(optionId), spread);
 
-    ) = call.writer.call{value: call.strike}("");
-    require(writerSent, "Failed to send Ether to option writer");
+    // send option writer the strike price
+    _safeTransferETHWithFallback(call.writer, call.strike);
 
     if (returnNft) {
       IHookERC721Vault(call.vaultAddress).withdrawalAsset();
@@ -401,8 +400,8 @@ contract HookCoveredCallImplV1 is
 
     if (call.highBidder != address(0)) {
       // return current bidder's money
-      (bool sent, ) = call.highBidder.call{value: call.bid}("");
-      require(sent, "Failed to send Ether");
+      _safeTransferETHWithFallback(call.highBidder, call.bid);
+
       // if we have a bid, we may have set the bidder, so make sure to revert it here.
       IHookERC721Vault(call.vaultAddress).setBeneficialOwner(call.writer);
     }
@@ -495,4 +494,25 @@ contract HookCoveredCallImplV1 is
 
     return output;
   }
+
+  /**
+   * @notice Transfer ETH. If the ETH transfer fails, wrap the ETH and try send it as WETH.
+   * @dev this transfer failure could occur if the transferee is a malicious contract
+   * so limiting the gas and persisting on fail helps prevent the impace of these calls.
+   */
+  function _safeTransferETHWithFallback(address to, uint256 amount) internal {
+    if (!_safeTransferETH(to, amount)) {
+      IWETH(weth).deposit{value: amount}();
+      IWETH(weth).transfer(to, amount);
+    }
+  }
+
+  /**
+   * @notice Transfer ETH and return the success status.
+   * @dev This function only forwards 30,000 gas to the callee.
+   */
+  function _safeTransferETH(address to, uint256 value) internal returns (bool) {
+    (bool success, ) = to.call{value: value, gas: 30_000}(new bytes(0));
+    return success;
+  }
 }

src/HookProtocol.sol

@@ -31,7 +31,15 @@ contract HookProtocol is
   /// @notice the address of the deployed VaultFactory used by the protocol
   address public override vaultContract;
 
-  constructor(address admin) {
+  /// @notice the standard weth address on this chain
+  /// @dev these are values for popular chains:
+  /// mainnet: 0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2
+  /// kovan: 0xd0a1e359811322d97991e03f863a0c30c2cf029c
+  /// ropsten: 0xc778417e063141139fce010982780140aa0cd5ab
+  /// rinkeby: 0xc778417e063141139fce010982780140aa0cd5ab
+  address public override getWETHAddress;
+
+  constructor(address admin, address weth) {
     _setupRole(ALLOWLISTER_ROLE, admin);
     _setupRole(PAUSER_ROLE, admin);
     _setupRole(VAULT_UPGRADER, admin);
@@ -42,6 +50,9 @@ contract HookProtocol is
     _setRoleAdmin(ALLOWLISTER_ROLE, ADMIN_ROLE);
     _setRoleAdmin(PAUSER_ROLE, ADMIN_ROLE);
     _setRoleAdmin(VAULT_UPGRADER, ADMIN_ROLE);
+
+    // set weth
+    getWETHAddress = weth;
   }
 
   modifier adminOnly() {

src/interfaces/IHookProtocol.sol

@@ -10,4 +10,6 @@ interface IHookProtocol is IAccessControl {
   function vaultContract() external view returns (address);
 
   function throwWhenPaused() external;
+
+  function getWETHAddress() external view returns (address);
 }

src/interfaces/IWETH.sol

@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-3.0
+
+pragma solidity ^0.8.10;
+
+interface IWETH {
+  function deposit() external payable;
+
+  function withdraw(uint256 wad) external;
+
+  function transfer(address to, uint256 value) external returns (bool);
+}

src/test/HookCoveredCallBiddingRevertTests.sol

@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity ^0.8.10;
+
+import "./utils/base.sol";
+import "./utils/mocks/MaliciousBidder.sol";
+
+// @dev these tests try cases where a bidder maliciously reverts on save.
+contract HookCoveredCallBiddingReverTests is HookProtocolTest {
+  function setUp() public {
+    setUpAddresses();
+    setUpFullProtocol();
+
+    // add address to the allowlist for minting
+
+    // Set user balances
+    vm.deal(address(buyer), 100 ether);
+
+    // Mint underlying token
+    underlyingTokenId = 0;
+    token.mint(address(writer), underlyingTokenId);
+
+    // Buyer swap 50 ETH <> 50 WETH
+    vm.prank(address(buyer));
+    weth.deposit{value: 50 ether}();
+
+    // Seller approve ERC721TransferHelper
+    vm.prank(address(writer));
+    token.setApprovalForAll(address(calls), true);
+
+    // Buyer approve covered call
+    vm.prank(address(buyer));
+    weth.approve(address(calls), 50 ether);
+  }
+
+  function test_SuccessfulAuctionAndSettlement() public {
+    // create the call option
+    vm.prank(address(writer));
+    uint256 writerStartBalance = writer.balance;
+    uint256 baseTime = block.timestamp;
+    uint256 expiration = baseTime + 3 days;
+    uint256 optionId = calls.mint(
+      address(token),
+      underlyingTokenId,
+      1000,
+      expiration,
+      makeSignature(underlyingTokenId, expiration, writer)
+    );
+    vm.prank(address(writer));
+    // assume that the writer somehow sold to the buyer, outside the scope of this test
+    calls.safeTransferFrom(writer, buyer, optionId);
+    uint256 buyerStartBalance = buyer.balance;
+
+    // create some bidders
+    MaliciousBidder bidder1 = new MaliciousBidder(address(calls));
+    address mbcaller = address(6969420);
+    address bidder2 = address(33456463);
+
+    // made a bid
+    vm.warp(baseTime + 2.1 days);
+    vm.deal(mbcaller, 1100);
+    vm.prank(mbcaller);
+    bidder1.bid{value: 1050}(optionId);
+
+    // validate that bid is updated
+    assertTrue(
+      calls.currentBid(optionId) == 1050,
+      "contract should update the current high bid for the option"
+    );
+    assertTrue(
+      calls.currentBidder(optionId) == address(bidder1),
+      "bidder1 should be in the lead"
+    );
+    assertTrue(
+      address(calls).balance == 1050,
+      "bidder1 should have deposited money into escrow"
+    );
+
+    // make a competing bid
+    vm.deal(bidder2, 1100);
+    vm.prank(bidder2);
+    calls.bid{value: 1100}(optionId);
+
+    // validate that bid is updated
+    assertTrue(
+      calls.currentBid(optionId) == 1100,
+      "contract should update the current high bid for the option"
+    );
+    assertTrue(
+      calls.currentBidder(optionId) == bidder2,
+      "bidder2 should be in the lead"
+    );
+    assertTrue(bidder2.balance == 0, "bidder2 should have funds in escrow");
+
+    // settle the auction
+    // assertTrue(token.ownerOf(underlyingTokenId) == address(calls), "call contract should own the token");
+    vm.warp(expiration + 3 seconds);
+    calls.settleOption(optionId, true);
+
+    // verify the balances are correct
+    uint256 writerEndBalance = writer.balance;
+    uint256 buyerEndBalance = buyer.balance;
+
+    assertTrue(
+      token.ownerOf(underlyingTokenId) == bidder2,
+      "the high bidder should own the nft"
+    );
+    assertTrue(
+      writerEndBalance - writerStartBalance == 1000,
+      "the writer gets the strike price"
+    );
+    assertTrue(
+      buyerEndBalance - buyerStartBalance == 100,
+      "the call owner gets the spread"
+    );
+  }
+}

src/test/utils/base.sol

@@ -65,7 +65,8 @@ contract HookProtocolTest is Test, EIP712, PermissionConstants {
   }
 
   function setUpFullProtocol() public {
-    protocol = new HookProtocol(admin);
+    weth = new WETH();
+    protocol = new HookProtocol(admin, address(weth));
     protocolAddress = address(protocol);
     setAddressForEipDomain(protocolAddress);
 

src/test/utils/mocks/MaliciousBidder.sol

@@ -0,0 +1,25 @@
+pragma solidity ^0.8.10;
+
+import "../../../interfaces/IHookCoveredCall.sol";
+
+// @dev a smart contract that reverts upon recieveing funds
+// and allows a bid to be mocked to a specific covered call option.
+// this can be used to write tests that fail if a contract reverting
+// prevents new bids.
+contract MaliciousBidder {
+  IHookCoveredCall private callOption;
+  bool private throwOnReceive;
+
+  constructor(address _callOption) {
+    callOption = IHookCoveredCall(_callOption);
+    throwOnReceive = true;
+  }
+
+  function bid(uint256 optionId) public payable {
+    callOption.bid{value: msg.value}(optionId);
+  }
+
+  receive() external payable {
+    require(!throwOnReceive, "ha ha ha gotcha");
+  }
+}