Add handling for code-423n4/2022-10-holograph-findings#456

contracts/enforcer/HolographERC721.sol

@@ -259,7 +259,7 @@ contract HolographERC721 is Admin, Owner, HolographERC721Interface, Initializabl
       (bool success, bytes memory returnData) = _royalties().delegatecall(
         abi.encodeWithSelector(
           HolographRoyaltiesInterface.initHolographRoyalties.selector,
-          abi.encode(uint256(contractBps))
+          abi.encode(uint256(contractBps), uint256(0))
         )
       );
       bytes4 selector = abi.decode(returnData, (bytes4));

contracts/enforcer/HolographRoyalties.sol

@@ -139,6 +139,11 @@ contract HolographRoyalties is Admin, Owner, Initializable {
    * @dev bytes32(uint256(keccak256('eip1967.Holograph.ROYALTIES.payout.bps')) - 1)
    */
   bytes32 constant _payoutBpsSlot = 0xa4023567d5b5f01c63e00d90785d7cd4ff057a237ad185c5ecade8f4059fb61a;
+  /**
+   * @dev bytes32(uint256(keccak256('eip1967.Holograph.ROYALTIES.extendedCall')) - 1)
+   * @dev extendedCall is an init config used to determine if payouts should be sent via a call or a transfer.
+   */
+  bytes32 constant _extendedCallSlot = 0x254926eafdecb56f669f96a3a752fbca17becd902481431df613768b1f903fa3;
 
   string constant _bpString = "eip1967.Holograph.ROYALTIES.bp";
   string constant _receiverString = "eip1967.Holograph.ROYALTIES.receiver";
@@ -149,7 +154,8 @@ contract HolographRoyalties is Admin, Owner, Initializable {
    * @dev Emits event in order to comply with Rarible V1 royalty spec.
    * @param tokenId Specific token id for which royalty info is being set, set as 0 for all tokens inside of the smart contract.
    * @param recipients Address array of wallets that will receive tha royalties.
-   * @param bps Uint256 array of base points(percentages) that each wallet(specified in recipients) will receive from the royalty payouts. Make sure that all the base points add up to a total of 10000.
+   * @param bps Uint256 array of base points(percentages) that each wallet(specified in recipients) will receive from the royalty payouts.
+   *            Make sure that all the base points add up to a total of 10000.
    */
   event SecondarySaleFees(uint256 tokenId, address[] recipients, uint256[] bps);
 
@@ -189,7 +195,13 @@ contract HolographRoyalties is Admin, Owner, Initializable {
       initialized := sload(_initializedPaidSlot)
     }
     require(initialized == 0, "ROYALTIES: already initialized");
-    uint256 bp = abi.decode(initPayload, (uint256));
+    (uint256 bp, uint256 useExtenededCall) = abi.decode(initPayload, (uint256, uint256));
+    if (useExtenededCall > 0) {
+      useExtenededCall = 1;
+    }
+    assembly {
+      sstore(_extendedCallSlot, useExtenededCall)
+    }
     setRoyalties(0, payable(address(this)), bp);
     address payable[] memory addresses = new address payable[](1);
     addresses[0] = payable(getOwner());
@@ -390,25 +402,44 @@ contract HolographRoyalties is Admin, Owner, Initializable {
   }
 
   /**
-   * @dev Internal function that transfers ETH to all payout recipients.
+   * @dev Private function that transfers ETH to all payout recipients.
+   * @dev This contract is designed primarily to capture royalties, but is limited in payout logic.
+   * @dev This function uses a push payment model, where the contract pushes the ETH to the recipients.
+   * @dev The design is intended so that royalty distribution logic can be handled externally via payment distribution contracts.
+   * @dev The recommended usage for royalty structures that require more complex payout logic to multiple recipients is to
+   *      set 100% ownership payout with the recipient being the payment distribution contract.
    */
   function _payoutEth() private {
     address payable[] memory addresses = _getPayoutAddresses();
     uint256[] memory bps = _getPayoutBps();
     uint256 length = addresses.length;
     uint256 balance = address(this).balance;
-    require(balance > 10000, "ROYALTIES: Not enough ETH");
     uint256 sending;
+    bool extendedCall;
+    assembly {
+      extendedCall := sload(_extendedCallSlot)
+    }
     for (uint256 i = 0; i < length; i++) {
       sending = ((bps[i] * balance) / 10000);
-      (bool success, ) = addresses[i].call{value: sending}("");
-      require(success, "ROYALTIES: Transfer failed");
+      // If the contract enabled extended call on init then use call to transfer, otherwise use transfer
+      if (extendedCall == true) {
+        (bool success, ) = addresses[i].call{value: sending}("");
+        require(success, "ROYALTIES: Transfer failed");
+      } else {
+        addresses[i].transfer(sending);
+      }
     }
   }
 
   /**
-   * @dev Internal function that transfers tokens to all payout recipients.
+   * @dev Private function that transfers tokens to all payout recipients.
    * @dev ERC20 tokens that use fee on transfer are not supported.
+   * @dev This contract is designed primarily to capture royalties, but is limited in payout logic.
+   * @dev This function uses a push payment model, where the contract pushes the ETH to the recipients.
+   * @dev The design is intended so that royalty distribution logic can be handled externally via payment distribution contracts.
+   * @dev The recommended usage for royalty structures that require more complex payout logic to multiple recipients is to
+   *      set 100% ownership payout with the recipient being the payment distribution contract.
+   *
    * @param tokenAddress Smart contract address of ERC20 token.
    */
   function _payoutToken(address tokenAddress) private {
@@ -423,18 +454,28 @@ contract HolographRoyalties is Admin, Owner, Initializable {
       // Some tokens revert when transferring a zero value amount this check ensures if one recipient's
       // amount is zero, the transfer will still succeed for the other recipients.
       if (sending > 0) {
-        // TODO: Need to use safeTransferFrom to support non-compliant tokens such as USDT, BNB, etc.
-        // See: https://github.com/code-423n4/2022-10-holograph-findings/issues/456
-        (bool success, ) = addresses[i].call{value: sending}("");
-        require(success, "ROYALTIES: ERC20 transfer failed");
+        require(
+          _callOptionalReturn(
+            tokenAddress,
+            erc20.transfer.selector,
+            abi.encode(address(addresses[i]), uint256(sending))
+          ),
+          "ROYALTIES: ERC20 transfer failed"
+        );
       }
     }
   }
 
   /**
-   * @dev Internal function that transfers multiple tokens to all payout recipients.
+   * @dev Private function that transfers multiple tokens to all payout recipients.
    * @dev Try to use _payoutToken and handle each token individually.
    * @dev ERC20 tokens that use fee on transfer are not supported.
+   * @dev This contract is designed primarily to capture royalties, but is limited in payout logic.
+   * @dev This function uses a push payment model, where the contract pushes the ETH to the recipients.
+   * @dev The design is intended so that royalty distribution logic can be handled externally via payment distribution contracts.
+   * @dev The recommended usage for royalty structures that require more complex payout logic to multiple recipients is to
+   *      set 100% ownership payout with the recipient being the payment distribution contract.
+   *
    * @param tokenAddresses Array of smart contract addresses of ERC20 tokens.
    */
   function _payoutTokens(address[] memory tokenAddresses) private {
@@ -451,10 +492,14 @@ contract HolographRoyalties is Admin, Owner, Initializable {
         // Some tokens revert when transferring a zero value amount this check ensures if one recipient's
         // amount is zero, the transfer will still succeed for the other recipients.
         if (sending > 0) {
-          // TODO: Need to use safeTransferFrom to support non-compliant tokens such as USDT, BNB, etc.
-          // See: https://github.com/code-423n4/2022-10-holograph-findings/issues/456
-          (bool success, ) = addresses[i].call{value: sending}("");
-          require(success, "ROYALTIES: ERC20 transfer failed");
+          require(
+            _callOptionalReturn(
+              tokenAddresses[t],
+              erc20.transfer.selector,
+              abi.encode(address(addresses[i]), uint256(sending))
+            ),
+            "ROYALTIES: ERC20 transfer failed"
+          );
         }
       }
     }
@@ -704,4 +749,31 @@ contract HolographRoyalties is Admin, Owner, Initializable {
   function getTokenAddress(string memory tokenName) public view returns (address) {
     return _getTokenAddress(tokenName);
   }
+
+  /**
+   * @notice Used to wrap function calls to check if they return without revert regardless of return type.
+   * @dev
+   * @return Returns true if the wrapped function call returns without a revert even if it doesn't return true.
+   */
+  function _callOptionalReturn(
+    address target,
+    bytes4 functionSignature,
+    bytes memory payload
+  ) internal returns (bool) {
+    bytes memory data = abi.encodePacked(functionSignature, payload);
+    bool success = true;
+    assembly {
+      let result := call(gas(), target, callvalue(), add(data, 0x20), mload(data), 0, 0)
+      switch result
+      case 0 {
+        revert(0, returndatasize())
+      }
+      default {
+        if gt(returndatasize(), 0) {
+          returndatacopy(success, 0, 0x20)
+        }
+      }
+    }
+    return success;
+  }
 }

deploy/03_interfaces.ts

@@ -284,7 +284,7 @@ const func: DeployFunction = async function (hre1: HardhatRuntimeEnvironment) {
     // HolographRoyalties
     '4': [
       // HolographRoyalties
-      functionHash('initHolographRoyalties(bytes)'),
+      functionHash('initHolographRoyalties(bytes,bytes)'),
       functionHash('configurePayouts(address[],uint256[])'),
       functionHash('getPayoutInfo()'),
       functionHash('getEthPayout()'),

src/enforcer/HolographERC721.sol

@@ -160,7 +160,7 @@ contract HolographERC721 is Admin, Owner, HolographERC721Interface, Initializabl
       (bool success, bytes memory returnData) = _royalties().delegatecall(
         abi.encodeWithSelector(
           HolographRoyaltiesInterface.initHolographRoyalties.selector,
-          abi.encode(uint256(contractBps))
+          abi.encode(uint256(contractBps), uint256(0))
         )
       );
       bytes4 selector = abi.decode(returnData, (bytes4));

src/enforcer/HolographRoyalties.sol

@@ -97,8 +97,8 @@ contract HolographRoyalties is Admin, Owner, Initializable {
     }
     require(initialized == 0, "ROYALTIES: already initialized");
     (uint256 bp, uint256 useExtenededCall) = abi.decode(initPayload, (uint256, uint256));
-    if (useExtendedCall > 0) {
-      useExtendedCall = 1;
+    if (useExtenededCall > 0) {
+      useExtenededCall = 1;
     }
     assembly {
       sstore(_extendedCallSlot, useExtenededCall)
@@ -303,7 +303,7 @@ contract HolographRoyalties is Admin, Owner, Initializable {
   }
 
   /**
-   * @dev Internal function that transfers ETH to all payout recipients.
+   * @dev Private function that transfers ETH to all payout recipients.
    * @dev This contract is designed primarily to capture royalties, but is limited in payout logic.
    * @dev This function uses a push payment model, where the contract pushes the ETH to the recipients.
    * @dev The design is intended so that royalty distribution logic can be handled externally via payment distribution contracts.
@@ -322,6 +322,7 @@ contract HolographRoyalties is Admin, Owner, Initializable {
     }
     for (uint256 i = 0; i < length; i++) {
       sending = ((bps[i] * balance) / 10000);
+      // If the contract enabled extended call on init then use call to transfer, otherwise use transfer
       if (extendedCall == true) {
         (bool success, ) = addresses[i].call{value: sending}("");
         require(success, "ROYALTIES: Transfer failed");
@@ -332,7 +333,7 @@ contract HolographRoyalties is Admin, Owner, Initializable {
   }
 
   /**
-   * @dev Internal function that transfers tokens to all payout recipients.
+   * @dev Private function that transfers tokens to all payout recipients.
    * @dev ERC20 tokens that use fee on transfer are not supported.
    * @dev This contract is designed primarily to capture royalties, but is limited in payout logic.
    * @dev This function uses a push payment model, where the contract pushes the ETH to the recipients.
@@ -354,16 +355,20 @@ contract HolographRoyalties is Admin, Owner, Initializable {
       // Some tokens revert when transferring a zero value amount this check ensures if one recipient's
       // amount is zero, the transfer will still succeed for the other recipients.
       if (sending > 0) {
-        // TODO: Need to use safeTransferFrom to support non-compliant tokens such as USDT, BNB, etc.
-        // Vitto mentioned that has has a way to handle this without safeTransferFrom using assembly.
-        // See: https://github.com/code-423n4/2022-10-holograph-findings/issues/456
-        require(erc20.transfer(addresses[i], sending), "ROYALTIES: ERC20 transfer failed");
+        require(
+          _callOptionalReturn(
+            tokenAddress,
+            erc20.transfer.selector,
+            abi.encode(address(addresses[i]), uint256(sending))
+          ),
+          "ROYALTIES: ERC20 transfer failed"
+        );
       }
     }
   }
 
   /**
-   * @dev Internal function that transfers multiple tokens to all payout recipients.
+   * @dev Private function that transfers multiple tokens to all payout recipients.
    * @dev Try to use _payoutToken and handle each token individually.
    * @dev ERC20 tokens that use fee on transfer are not supported.
    * @dev This contract is designed primarily to capture royalties, but is limited in payout logic.
@@ -388,10 +393,14 @@ contract HolographRoyalties is Admin, Owner, Initializable {
         // Some tokens revert when transferring a zero value amount this check ensures if one recipient's
         // amount is zero, the transfer will still succeed for the other recipients.
         if (sending > 0) {
-          // TODO: Need to use safeTransferFrom to support non-compliant tokens such as USDT, BNB, etc.
-          // Vitto mentioned that has has a way to handle this without safeTransferFrom using assembly.
-          // See: https://github.com/code-423n4/2022-10-holograph-findings/issues/456
-          require(erc20.transfer(addresses[i], sending), "ROYALTIES: ERC20 transfer failed");
+          require(
+            _callOptionalReturn(
+              tokenAddresses[t],
+              erc20.transfer.selector,
+              abi.encode(address(addresses[i]), uint256(sending))
+            ),
+            "ROYALTIES: ERC20 transfer failed"
+          );
         }
       }
     }
@@ -641,4 +650,31 @@ contract HolographRoyalties is Admin, Owner, Initializable {
   function getTokenAddress(string memory tokenName) public view returns (address) {
     return _getTokenAddress(tokenName);
   }
+
+  /**
+   * @notice Used to wrap function calls to check if they return without revert regardless of return type.
+   * @dev
+   * @return Returns true if the wrapped function call returns without a revert even if it doesn't return true.
+   */
+  function _callOptionalReturn(
+    address target,
+    bytes4 functionSignature,
+    bytes memory payload
+  ) internal returns (bool) {
+    bytes memory data = abi.encodePacked(functionSignature, payload);
+    bool success = true;
+    assembly {
+      let result := call(gas(), target, callvalue(), add(data, 0x20), mload(data), 0, 0)
+      switch result
+      case 0 {
+        revert(0, returndatasize())
+      }
+      default {
+        if gt(returndatasize(), 0) {
+          returndatacopy(success, 0, 0x20)
+        }
+      }
+    }
+    return success;
+  }
 }