Merge pull request #1320 from keep-network/remove-operator-trust-service-contract

Operator callback reimbursment in the operator contract

Author: ngrinkevich

contracts/solidity/contracts/KeepRandomBeaconOperator.sol

@@ -8,6 +8,7 @@ import "./utils/AddressArrayUtils.sol";
 import "./libraries/operator/GroupSelection.sol";
 import "./libraries/operator/Groups.sol";
 import "./libraries/operator/DKGResultVerification.sol";
+import "./libraries/operator/Reimbursements.sol";
 
 interface ServiceContract {
     function entryCreated(uint256 requestId, bytes calldata entry, address payable submitter) external;
@@ -95,8 +96,9 @@ contract KeepRandomBeaconOperator is ReentrancyGuard {
     uint256 public relayEntryTimeout = relayEntryGenerationTime.add(groupSize.mul(resultPublicationBlockStep));
 
     // Gas required to verify BLS signature and produce successful relay
-    // entry. Excludes callback and DKG gas.
-    uint256 public entryVerificationGasEstimate = 240000;
+    // entry. Excludes callback and DKG gas. The worst case (most expensive)
+    // scenario.
+    uint256 public entryVerificationGasEstimate = 280000;
 
     // Gas required to submit DKG result. Excludes initiation of group selection.
     uint256 public dkgGasEstimate = 1740000;
@@ -118,6 +120,7 @@ contract KeepRandomBeaconOperator is ReentrancyGuard {
     struct SigningRequest {
         uint256 relayRequestId;
         uint256 entryVerificationAndProfitFee;
+        uint256 callbackFee;
         uint256 groupIndex;
         bytes previousEntry;
         address serviceContract;
@@ -371,17 +374,15 @@ contract KeepRandomBeaconOperator is ReentrancyGuard {
             uint256 surplus = dkgSubmitterReimbursementFee.sub(reimbursementFee);
             dkgSubmitterReimbursementFee = 0;
             // Reimburse submitter with actual DKG cost.
-            (bool success, ) = magpie.call.value(reimbursementFee)("");
-            require(success, "Failed reimburse actual DKG cost");
+            magpie.call.value(reimbursementFee)("");
 
             // Return surplus to the contract that started DKG.
             groupSelectionStarterContract.fundDkgFeePool.value(surplus)();
         } else {
             // If submitter used higher gas price reimburse only dkgSubmitterReimbursementFee max.
             reimbursementFee = dkgSubmitterReimbursementFee;
             dkgSubmitterReimbursementFee = 0;
-            (bool success, ) = magpie.call.value(reimbursementFee)("");
-            require(success, "Failed reimburse DKG fee");
+            magpie.call.value(reimbursementFee)("");
         }
     }
 
@@ -403,18 +404,26 @@ contract KeepRandomBeaconOperator is ReentrancyGuard {
         uint256 requestId,
         bytes memory previousEntry
     ) public payable onlyServiceContract {
+        uint256 entryVerificationAndProfitFee = groupProfitFee().add(
+            entryVerificationGasEstimate.mul(gasPriceWithFluctuationMargin(priceFeedEstimate))
+        );
         require(
-            msg.value >= groupProfitFee().add(entryVerificationGasEstimate.mul(gasPriceWithFluctuationMargin(priceFeedEstimate))),
+            msg.value >= entryVerificationAndProfitFee,
             "Insufficient new entry fee"
         );
-        signRelayEntry(requestId, previousEntry, msg.sender, msg.value);
+        uint256 callbackFee = msg.value.sub(entryVerificationAndProfitFee);
+        signRelayEntry(
+            requestId, previousEntry, msg.sender,
+            entryVerificationAndProfitFee, callbackFee
+        );
     }
 
     function signRelayEntry(
         uint256 requestId,
         bytes memory previousEntry,
         address serviceContract,
-        uint256 entryVerificationAndProfitFee
+        uint256 entryVerificationAndProfitFee,
+        uint256 callbackFee
     ) internal {
         require(!isEntryInProgress() || hasEntryTimedOut(), "Beacon is busy");
 
@@ -424,6 +433,7 @@ contract KeepRandomBeaconOperator is ReentrancyGuard {
         signingRequest = SigningRequest(
             requestId,
             entryVerificationAndProfitFee,
+            callbackFee,
             groupIndex,
             previousEntry,
             serviceContract
@@ -455,25 +465,60 @@ contract KeepRandomBeaconOperator is ReentrancyGuard {
 
         emit RelayEntrySubmitted();
 
-        ServiceContract(signingRequest.serviceContract).entryCreated(
-            signingRequest.relayRequestId,
-            _groupSignature,
-            msg.sender
+        // Spend no more than groupSelectionGasEstimate + 40000 gas max
+        // This will prevent relayEntry failure in case the service contract is compromised
+        signingRequest.serviceContract.call.gas(groupSelectionGasEstimate.add(40000))(
+            abi.encodeWithSignature(
+                "entryCreated(uint256,bytes,address)",
+                signingRequest.relayRequestId,
+                _groupSignature,
+                msg.sender
+            )
         );
 
+        if (signingRequest.callbackFee > 0) {
+            executeCallback(signingRequest, uint256(keccak256(_groupSignature)));
+        }
+
         (uint256 groupMemberReward, uint256 submitterReward, uint256 subsidy) = newEntryRewardsBreakdown();
         groups.addGroupMemberReward(groupPubKey, groupMemberReward);
 
-        (bool success, ) = stakingContract.magpieOf(msg.sender).call.value(submitterReward)("");
-        require(success, "Failed send relay submitter reward");
+        stakingContract.magpieOf(msg.sender).call.value(submitterReward)("");
 
         if (subsidy > 0) {
-            ServiceContract(signingRequest.serviceContract).fundRequestSubsidyFeePool.value(subsidy)();
+            signingRequest.serviceContract.call.gas(35000).value(subsidy)(abi.encodeWithSignature("fundRequestSubsidyFeePool()"));
         }
 
         currentEntryStartBlock = 0;
     }
 
+    /**
+     * @dev Executes customer specified callback for the relay entry request.
+     * @param signingRequest Request data tracked internally by this contract.
+     * @param entry The generated random number.
+     */
+    function executeCallback(SigningRequest memory signingRequest, uint256 entry) internal {
+        uint256 callbackFee = signingRequest.callbackFee;
+
+        // Make sure not to spend more than what was received from the service contract for the callback
+        uint256 gasLimit = callbackFee.div(gasPriceWithFluctuationMargin(priceFeedEstimate));
+
+        bytes memory callbackReturnData;
+        uint256 gasBeforeCallback = gasleft();
+        (, callbackReturnData) = signingRequest.serviceContract.call.gas(gasLimit)(abi.encodeWithSignature("executeCallback(uint256,uint256)", signingRequest.relayRequestId, entry));
+        uint256 gasAfterCallback = gasleft();
+        uint256 gasSpent = gasBeforeCallback.sub(gasAfterCallback);
+
+        Reimbursements.reimburseCallback(
+            stakingContract,
+            priceFeedEstimate,
+            gasLimit,
+            gasSpent,
+            callbackFee,
+            callbackReturnData
+        );
+    }
+
     /**
      * @dev Get rewards breakdown in wei for successful entry for the current signing request.
      */
@@ -574,7 +619,8 @@ contract KeepRandomBeaconOperator is ReentrancyGuard {
                 signingRequest.relayRequestId,
                 signingRequest.previousEntry,
                 signingRequest.serviceContract,
-                signingRequest.entryVerificationAndProfitFee
+                signingRequest.entryVerificationAndProfitFee,
+                signingRequest.callbackFee
             );
         }
     }
@@ -659,8 +705,9 @@ contract KeepRandomBeaconOperator is ReentrancyGuard {
     function withdrawGroupMemberRewards(address operator, uint256 groupIndex, uint256[] memory groupMemberIndices) public nonReentrant {
         uint256 accumulatedRewards = groups.withdrawFromGroup(operator, groupIndex, groupMemberIndices);
         (bool success, ) = stakingContract.magpieOf(operator).call.value(accumulatedRewards)("");
-        require(success, "Failed withdraw rewards");
-        emit GroupMemberRewardsWithdrawn(stakingContract.magpieOf(operator), operator, accumulatedRewards, groupIndex);
+        if (success) {
+            emit GroupMemberRewardsWithdrawn(stakingContract.magpieOf(operator), operator, accumulatedRewards, groupIndex);
+        }
     }
 
     /**

contracts/solidity/contracts/KeepRandomBeaconServiceImplV1.sol

@@ -66,6 +66,9 @@ contract KeepRandomBeaconServiceImplV1 is DelayedWithdrawal, ReentrancyGuard {
 
     bytes internal _previousEntry;
 
+    // Cost of executing executeCallback() on this contract, require checks and .call
+    uint256 internal _baseCallbackGas;
+
     struct Callback {
         address callbackContract;
         string callbackMethod;
@@ -130,6 +133,7 @@ contract KeepRandomBeaconServiceImplV1 is DelayedWithdrawal, ReentrancyGuard {
         _pendingWithdrawal = 0;
         _previousEntry = _beaconSeed;
         _registry = registry;
+        _baseCallbackGas = 18845;
     }
 
     /**
@@ -261,7 +265,7 @@ contract KeepRandomBeaconServiceImplV1 is DelayedWithdrawal, ReentrancyGuard {
         uint256 requestId = _requestCounter;
 
         operatorContract.sign.value(
-            selectedOperatorContractFee
+            selectedOperatorContractFee.add(callbackFee)
         )(requestId, _previousEntry);
 
         // If selected operator contract is cheaper than expected return the
@@ -304,59 +308,30 @@ contract KeepRandomBeaconServiceImplV1 is DelayedWithdrawal, ReentrancyGuard {
         uint256 entryAsNumber = uint256(keccak256(entry));
         emit RelayEntryGenerated(requestId, entryAsNumber);
 
-        if (_callbacks[requestId].callbackContract != address(0)) {
-            executeEntryCreatedCallback(requestId, entryAsNumber, submitter);
-            delete _callbacks[requestId];
-        }
-
         createGroupIfApplicable(entryAsNumber, submitter);
     }
 
     /**
      * @dev Executes customer specified callback for the relay entry request.
      * @param requestId Request id tracked internally by this contract.
      * @param entry The generated random number.
-     * @param submitter Relay entry submitter.
+     * @return Address to receive callback surplus.
      */
-    function executeEntryCreatedCallback(uint256 requestId, uint256 entry, address payable submitter) internal {
-        bool success; // Store status of external contract call.
-        bytes memory data; // Store result data of external contract call.
-
-        uint256 gasBeforeCallback = gasleft();
-        (success, data) = _callbacks[requestId].callbackContract.call.gas(
-            _callbacks[requestId].callbackGas
-        )(abi.encodeWithSignature(_callbacks[requestId].callbackMethod, entry));
-        uint256 gasSpent = gasBeforeCallback.sub(gasleft()).add(21000); // Also reimburse 21000 gas (ethereum transaction minimum gas)
-
-        uint256 gasPrice = _priceFeedEstimate;
-        // We need to check if tx.gasprice is non-zero as a workaround to a bug
-        // in go-ethereum:
-        // https://github.com/ethereum/go-ethereum/pull/20189
-        if (tx.gasprice > 0 && tx.gasprice < _priceFeedEstimate) {
-            gasPrice = tx.gasprice;
-        }
+    function executeCallback(uint256 requestId, uint256 entry) public returns (address payable surplusRecipient) {
+        require(
+            _operatorContracts.contains(msg.sender),
+            "Only authorized operator contract can call execute callback."
+        );
 
-        // Obtain the actual callback gas expenditure and refund the surplus.
-        uint256 callbackSurplus = 0;
-        uint256 callbackFee = gasSpent.mul(gasPrice);
-
-        // If we spent less on the callback than the customer transferred for the
-        // callback execution, we need to reimburse the difference.
-        if (callbackFee < _callbacks[requestId].callbackFee) {
-            callbackSurplus = _callbacks[requestId].callbackFee.sub(callbackFee);
-            // Reimburse submitter with his actual callback cost.
-            (success, ) = submitter.call.value(callbackFee)("");
-            require(success, "Failed reimburse actual callback cost");
-
-            // Return callback surplus to the requestor.
-            (success, ) = _callbacks[requestId].surplusRecipient.call.value(callbackSurplus)("");
-            require(success, "Failed send callback surplus");
-
-        } else {
-            // Reimburse submitter with the callback payment sent by the requestor.
-            (success, ) = submitter.call.value(_callbacks[requestId].callbackFee)("");
-            require(success, "Failed reimburse callback payment");
-        }
+        require(
+            _callbacks[requestId].callbackContract != address(0),
+            "Callback contract not found"
+        );
+
+        _callbacks[requestId].callbackContract.call(abi.encodeWithSignature(_callbacks[requestId].callbackMethod, entry));
+
+        surplusRecipient = _callbacks[requestId].surplusRecipient;
+        delete _callbacks[requestId];
     }
 
     /**
@@ -377,6 +352,13 @@ contract KeepRandomBeaconServiceImplV1 is DelayedWithdrawal, ReentrancyGuard {
         }
     }
 
+    /**
+     * @dev Get base callback gas required for relay entry callback.
+     */
+    function baseCallbackGas() public view returns(uint256) {
+        return _baseCallbackGas;
+    }
+
     /**
      * @dev Set the gas price in wei for estimating relay entry request payment.
      * @param priceFeedEstimate is the gas price required for estimating relay entry request payment.
@@ -407,9 +389,12 @@ contract KeepRandomBeaconServiceImplV1 is DelayedWithdrawal, ReentrancyGuard {
     /**
      * @dev Get the minimum payment in wei for relay entry callback.
      * The returned value includes safety margin for gas price fluctuations.
-     * @param callbackGas Gas required for the callback.
+     * @param _callbackGas Gas required for the callback.
      */
-    function callbackFee(uint256 callbackGas) public view returns(uint256) {
+    function callbackFee(uint256 _callbackGas) public view returns(uint256) {
+        // gas for the callback itself plus additional operational costs of
+        // executing the callback
+        uint256 callbackGas = _callbackGas == 0 ? 0 : _callbackGas.add(_baseCallbackGas);
         // We take the gas price from the price feed to not let malicious
         // miner-requestors manipulate the gas price when requesting relay entry
         // and underpricing expensive callbacks.

contracts/solidity/contracts/libraries/operator/Reimbursements.sol

@@ -0,0 +1,61 @@
+pragma solidity ^0.5.4;
+
+import "openzeppelin-solidity/contracts/math/SafeMath.sol";
+import "solidity-bytes-utils/contracts/BytesLib.sol";
+import "../../TokenStaking.sol";
+
+library Reimbursements {
+    using SafeMath for uint256;
+    using BytesLib for bytes;
+
+    /**
+     * @dev Reimburses callback execution cost and surplus based on actual gas
+     * usage to the submitter's beneficiary address and if necessary to the
+     * callback requestor (surplus recipient).
+     * @param stakingContract Staking contract to get the address of the beneficiary
+     * @param gasLimit Gas limit set for the callback
+     * @param gasSpent Gas spent by the submitter on the callback
+     * @param callbackFee Fee paid for the callback by the requestor
+     * @param callbackReturnData Data containing surplus recipient address
+     */
+    function reimburseCallback(
+        TokenStaking stakingContract,
+        uint256 priceFeedEstimate,
+        uint256 gasLimit,
+        uint256 gasSpent,
+        uint256 callbackFee,
+        bytes memory callbackReturnData
+    ) public {
+        uint256 gasPrice = tx.gasprice < priceFeedEstimate ? tx.gasprice : priceFeedEstimate;
+
+        // Obtain the actual callback gas expenditure and refund the surplus.
+        //
+        // In case of heavily underpriced transactions, EVM may wrap the call
+        // with additional opcodes. In this case gasSpent > gasLimit.
+        // The worst scenario cost is included in entry verification fee.
+        // If this happens we return just the gasLimit here.
+        uint256 actualCallbackGas = gasSpent < gasLimit ? gasSpent : gasLimit;
+        uint256 actualCallbackFee = actualCallbackGas.mul(gasPrice);
+
+        // Get the beneficiary.
+        address payable magpie = stakingContract.magpieOf(msg.sender);
+
+        // If we spent less on the callback than the customer transferred for the
+        // callback execution, we need to reimburse the difference.
+        if (actualCallbackFee < callbackFee) {
+            uint256 callbackSurplus = callbackFee.sub(actualCallbackFee);
+            // Reimburse submitter with his actual callback cost.
+            magpie.call.value(actualCallbackFee)("");
+
+            // Return callback surplus to the requestor.
+            // Expecting 32 bytes data containing 20 byte address
+            if (callbackReturnData.length == 32) {
+                address surplusRecipient = callbackReturnData.toAddress(12);
+                surplusRecipient.call.gas(8000).value(callbackSurplus)("");
+            }
+        } else {
+            // Reimburse submitter with the callback payment sent by the requestor.
+            magpie.call.value(callbackFee)("");
+        }
+    }
+}