From 1a33d6d487fffb205cb3cb35f27892b44fbc9f6a Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 24 Jan 2024 01:32:56 -0800 Subject: [PATCH 1/5] added a payValidator method so that a validator can transfer their balances from one fastlane auction contract to a new, updated one without having to take custody of the funds --- .../FastLaneAuctionHandler.sol | 83 +++++++++++++++++-- 1 file changed, 76 insertions(+), 7 deletions(-) diff --git a/contracts/auction-handler/FastLaneAuctionHandler.sol b/contracts/auction-handler/FastLaneAuctionHandler.sol index 38e1aae..5bcc357 100644 --- a/contracts/auction-handler/FastLaneAuctionHandler.sol +++ b/contracts/auction-handler/FastLaneAuctionHandler.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.16; import {SafeTransferLib, ERC20} from "solmate/utils/SafeTransferLib.sol"; import {IPaymentProcessor} from "../interfaces/IPaymentProcessor.sol"; +import {IFastLaneAuctionHandler} from "../interfaces/IFastLaneAuctionHandler.sol"; abstract contract FastLaneAuctionHandlerEvents { event RelayValidatorPayeeUpdated(address validator, address payee, address indexed initiator); @@ -57,6 +58,15 @@ abstract contract FastLaneAuctionHandlerEvents { uint256 endBlock ); + event CustomPaymentProcessorReceived( + address indexed payor, + address indexed payee, + address indexed paymentProcessor, + uint256 totalAmount, + uint256 startBlock, + uint256 endBlock + ); + // NOTE: Investigated Validators should be presumed innocent. This event can be triggered inadvertently by honest validators // while building a block due to transaction nonces taking precedence over gasPrice. event RelayInvestigateOutcome( @@ -99,6 +109,7 @@ abstract contract FastLaneAuctionHandlerEvents { error RelayCustomPayoutCantBePartial(); error RelayUnapprovedReentrancy(); + error RelayNotCurrentValidator(); } /// @notice Validator Data Struct @@ -372,6 +383,50 @@ contract FastLaneAuctionHandler is FastLaneAuctionHandlerEvents { emit RelayFeeCollected(_payor, block.coinbase, msg.value); } + // Part of the PaymentProcessor interface. In this case, this is used to transfer a validator's balances from one + // PFL contract to a new deployment. + function payValidator( + address validator, + uint256 startBlock, + uint256 endBlock, + uint256 totalAmount, + bytes calldata + ) external nonReentrant { + if (endBlock != block.number || startBlock >= block.number) revert RelayUnapprovedReentrancy(); + if (validator == block.coinbase || msg.sender == block.coinbase || tx.origin == block.coinbase) { + // NOTE: Validators can arbitrarily set the block.coinbase. This should only be used to prevent + // withdraws and deposits / MEV payments to block.coinbase from occuring in the same block. + revert RelayNotCurrentValidator(); + } + if (totalAmount == 0) revert RelayCannotBeZero(); + + // Store the current balance, don't necessarily trust the caller. + uint256 balance = address(this).balance; + uint256 vTotal = validatorsTotal; + + // Callback into the previous contract + IFastLaneAuctionHandler(msg.sender).paymentCallback(validator, address(this), totalAmount); + + // Revert if balance hasn't been incremented by the expected amount. + if (address(this).balance != balance + totalAmount) revert RelayCustomPayoutCantBePartial(); + + // Make sure validator balances are unchanged to prevent double counting. + if (vTotal != validatorsTotal) revert RelayUnapprovedReentrancy(); + + // Increment the balances. + validatorsBalanceMap[validator] += totalAmount; + validatorsTotal += totalAmount; + + emit CustomPaymentProcessorReceived({ + payor: msg.sender, + payee: validator, + paymentProcessor: address(this), + totalAmount: totalAmount, + startBlock: 0, // 0 to indicate this was a transfer + endBlock: 0 // 0 to indicate this was a transfer + }); + } + /// @notice Submits a SIMULATED flash bid. THE HTTP RELAY won't accept calls for this function. /// @notice This is just a convenience function for you to test by simulating a call to simulateFlashBid /// @notice To ensure your calldata correctly works when relayed to `_searcherToAddress`.fastLaneCall(_searcherCallData) @@ -554,7 +609,7 @@ contract FastLaneAuctionHandler is FastLaneAuctionHandlerEvents { /// @dev Callable by either validator address or their payee address (if not changed recently). function collectFees() external nonReentrant validPayee returns (uint256) { // NOTE: Do not let validatorsBalanceMap[validator] balance go to 0, that will remove them from being an "active validator" - address _validator = getValidator(); + address _validator = getValidatorIfNotCurrent(); uint256 payableBalance = validatorsBalanceMap[_validator] - 1; if (payableBalance <= 0) revert RelayCannotBeZero(); @@ -575,7 +630,8 @@ contract FastLaneAuctionHandler is FastLaneAuctionHandlerEvents { { if (paymentProcessor == address(0) || paymentProcessor == address(this)) revert RelayProcessorCannotBeZero(); - address validator = getValidator(); + address validator = getValidatorIfNotCurrent(); + uint256 validatorBalance = validatorsBalanceMap[validator] - 1; IPaymentProcessor(paymentProcessor).payValidator({ @@ -649,11 +705,7 @@ contract FastLaneAuctionHandler is FastLaneAuctionHandlerEvents { /// @notice Updates a validator's share /// @param refundShare the share in % that should be paid to the validator function updateValidatorRefundShare(uint256 refundShare) public validPayee nonReentrant { - address validator = getValidator(); - - // ensure that validators can't insert txs to boost their refund rates during their own blocks - if (validator == block.coinbase) revert RelayImmutableBlockAuthorRate(); - + address validator = getValidatorIfNotCurrent(); validatorsRefundShareMap[validator] = int256(refundShare) - DEFAULT_VALIDATOR_REFUND_SHARE; } @@ -714,6 +766,23 @@ contract FastLaneAuctionHandler is FastLaneAuctionHandlerEvents { revert("Invalid validator"); } + function getValidatorIfNotCurrent() internal view returns (address validator) { + if (validatorsBalanceMap[msg.sender] > 0) { + validator = msg.sender; + } else if (payeeMap[msg.sender] != address(0)) { + validator = payeeMap[msg.sender]; + } else { + // throw if invalid + revert("Invalid validator"); + } + + if (validator == block.coinbase || msg.sender == block.coinbase || tx.origin == block.coinbase) { + // NOTE: Validators can arbitrarily set the block.coinbase. This should only be used to prevent + // withdraws and deposits from occuring in the same block. + revert RelayNotCurrentValidator(); + } + } + /** * | * | Modifiers | From 77a3293a35d464096e7c3d6f49bae1817803549c Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 24 Jan 2024 01:33:21 -0800 Subject: [PATCH 2/5] forge fmt --- .../FastLaneAuctionHandler.sol | 70 ++++++++----------- .../interfaces/IFastLaneAuctionHandler.sol | 9 ++- 2 files changed, 37 insertions(+), 42 deletions(-) diff --git a/contracts/auction-handler/FastLaneAuctionHandler.sol b/contracts/auction-handler/FastLaneAuctionHandler.sol index 5bcc357..bfa9e71 100644 --- a/contracts/auction-handler/FastLaneAuctionHandler.sol +++ b/contracts/auction-handler/FastLaneAuctionHandler.sol @@ -260,16 +260,15 @@ contract FastLaneAuctionHandler is FastLaneAuctionHandlerEvents { /// @notice Submits a fast bid /// @dev Will not revert /// @param fastGasPrice Bonus gasPrice rate that Searcher commits to pay to validator for gas used by searcher's call - /// @param executeOnLoss Boolean flag that enables Searcher calls to execute even if they lost the auction. + /// @param executeOnLoss Boolean flag that enables Searcher calls to execute even if they lost the auction. /// @param searcherToAddress Searcher contract address to be called on its `fastLaneCall` function. /// @param searcherCallData callData to be passed to `_searcherToAddress.fastLaneCall(fastPrice,msg.sender,callData)` function submitFastBid( uint256 fastGasPrice, // surplus gasprice commited to be paid at the end of execution bool executeOnLoss, // If true, execute even if searcher lost auction address searcherToAddress, - bytes calldata searcherCallData + bytes calldata searcherCallData ) external payable onlyEOA nonReentrant { - if (searcherToAddress == address(this) || searcherToAddress == msg.sender) revert RelaySearcherWrongParams(); PGAData memory existing_bid = fulfilledPGAMap[block.number]; @@ -279,30 +278,31 @@ contract FastLaneAuctionHandler is FastLaneAuctionHandlerEvents { bool alreadyPaid = existing_bid.paid; // NOTE: These checks help mitigate the damage to searchers caused by relay error and adversarial validators by reverting - // early if the transactions are not sequenced pursuant to auction rules. + // early if the transactions are not sequenced pursuant to auction rules. - // Do not execute if a fastBid tx with a lower gasPrice was executed prior to this tx in the same block. - // NOTE: This edge case should only be achieveable via validator manipulation or erratic searcher nonce management + // Do not execute if a fastBid tx with a lower gasPrice was executed prior to this tx in the same block. + // NOTE: This edge case should only be achieveable via validator manipulation or erratic searcher nonce management if (lowestGasPrice != 0 && lowestGasPrice < tx.gasprice) { - emit RelayInvestigateOutcome(block.coinbase, msg.sender, block.number, lowestFastPrice, fastGasPrice, lowestGasPrice, tx.gasprice); - - // Do not execute if a fastBid tx with a lower bid amount was executed prior to this tx in the same block. + emit RelayInvestigateOutcome( + block.coinbase, msg.sender, block.number, lowestFastPrice, fastGasPrice, lowestGasPrice, tx.gasprice + ); + + // Do not execute if a fastBid tx with a lower bid amount was executed prior to this tx in the same block. } else if (lowestTotalPrice != 0 && lowestTotalPrice <= fastGasPrice + tx.gasprice) { - emit RelayInvestigateOutcome(block.coinbase, msg.sender, block.number, lowestFastPrice, fastGasPrice, lowestGasPrice, tx.gasprice); - - // Execute the tx if there are no issues w/ ordering. - // Execute the tx if the searcher enabled executeOnLoss or if the searcher won - } else if (executeOnLoss || !alreadyPaid) { + emit RelayInvestigateOutcome( + block.coinbase, msg.sender, block.number, lowestFastPrice, fastGasPrice, lowestGasPrice, tx.gasprice + ); + // Execute the tx if there are no issues w/ ordering. + // Execute the tx if the searcher enabled executeOnLoss or if the searcher won + } else if (executeOnLoss || !alreadyPaid) { // Use a try/catch pattern so that tx.gasprice and bidAmount can be saved to verify that - // proper transaction ordering is being followed. - try this.fastBidWrapper{value: msg.value}( - msg.sender, fastGasPrice, searcherToAddress, searcherCallData - ) returns (uint256 bidAmount) { - + // proper transaction ordering is being followed. + try this.fastBidWrapper{value: msg.value}(msg.sender, fastGasPrice, searcherToAddress, searcherCallData) + returns (uint256 bidAmount) { // Mark this auction as being complete to provide quicker reverts for subsequent searchers fulfilledPGAMap[block.number] = PGAData({ - lowestGasPrice: uint64(tx.gasprice), + lowestGasPrice: uint64(tx.gasprice), lowestFastPrice: uint64(fastGasPrice), lowestTotalPrice: uint64(fastGasPrice + tx.gasprice), paid: true @@ -311,27 +311,22 @@ contract FastLaneAuctionHandler is FastLaneAuctionHandlerEvents { emit RelayFastBid(msg.sender, block.coinbase, true, bidAmount, searcherToAddress); return; // return early so that we don't refund the searcher's msg.value - } catch { // Update the auction to provide quicker reverts for subsequent searchers fulfilledPGAMap[block.number] = PGAData({ - lowestGasPrice: uint64(tx.gasprice), + lowestGasPrice: uint64(tx.gasprice), lowestFastPrice: uint64(fastGasPrice), lowestTotalPrice: uint64(fastGasPrice + tx.gasprice), paid: alreadyPaid // carry forward any previous wins in the block }); emit RelayFastBid(msg.sender, block.coinbase, false, 0, searcherToAddress); - } } if (msg.value > 0) { - // Refund the searcher any msg.value for failed txs. - SafeTransferLib.safeTransferETH( - msg.sender, - msg.value - ); + // Refund the searcher any msg.value for failed txs. + SafeTransferLib.safeTransferETH(msg.sender, msg.value); } } @@ -384,14 +379,11 @@ contract FastLaneAuctionHandler is FastLaneAuctionHandlerEvents { } // Part of the PaymentProcessor interface. In this case, this is used to transfer a validator's balances from one - // PFL contract to a new deployment. - function payValidator( - address validator, - uint256 startBlock, - uint256 endBlock, - uint256 totalAmount, - bytes calldata - ) external nonReentrant { + // PFL contract to a new deployment. + function payValidator(address validator, uint256 startBlock, uint256 endBlock, uint256 totalAmount, bytes calldata) + external + nonReentrant + { if (endBlock != block.number || startBlock >= block.number) revert RelayUnapprovedReentrancy(); if (validator == block.coinbase || msg.sender == block.coinbase || tx.origin == block.coinbase) { // NOTE: Validators can arbitrarily set the block.coinbase. This should only be used to prevent @@ -400,7 +392,7 @@ contract FastLaneAuctionHandler is FastLaneAuctionHandlerEvents { } if (totalAmount == 0) revert RelayCannotBeZero(); - // Store the current balance, don't necessarily trust the caller. + // Store the current balance, don't necessarily trust the caller. uint256 balance = address(this).balance; uint256 vTotal = validatorsTotal; @@ -410,7 +402,7 @@ contract FastLaneAuctionHandler is FastLaneAuctionHandlerEvents { // Revert if balance hasn't been incremented by the expected amount. if (address(this).balance != balance + totalAmount) revert RelayCustomPayoutCantBePartial(); - // Make sure validator balances are unchanged to prevent double counting. + // Make sure validator balances are unchanged to prevent double counting. if (vTotal != validatorsTotal) revert RelayUnapprovedReentrancy(); // Increment the balances. @@ -775,7 +767,7 @@ contract FastLaneAuctionHandler is FastLaneAuctionHandlerEvents { // throw if invalid revert("Invalid validator"); } - + if (validator == block.coinbase || msg.sender == block.coinbase || tx.origin == block.coinbase) { // NOTE: Validators can arbitrarily set the block.coinbase. This should only be used to prevent // withdraws and deposits from occuring in the same block. diff --git a/contracts/interfaces/IFastLaneAuctionHandler.sol b/contracts/interfaces/IFastLaneAuctionHandler.sol index ff634f7..dbea03c 100644 --- a/contracts/interfaces/IFastLaneAuctionHandler.sol +++ b/contracts/interfaces/IFastLaneAuctionHandler.sol @@ -85,9 +85,12 @@ interface IFastLaneAuctionHandler { address searcherToAddress, bytes memory searcherCallData ) external payable; - function submitFastBid(uint256 fastGasPrice, bool executeOnLoss, address searcherToAddress, bytes memory searcherCallData) - external - payable; + function submitFastBid( + uint256 fastGasPrice, + bool executeOnLoss, + address searcherToAddress, + bytes memory searcherCallData + ) external payable; function submitFlashBid( uint256 bidAmount, bytes32 oppTxHash, From 74c86b21d38418d94272e9110a859c02e9c2e664 Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 24 Jan 2024 01:41:28 -0800 Subject: [PATCH 3/5] added a generic function to pay a fee to a specific validator --- contracts/auction-handler/FastLaneAuctionHandler.sol | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/contracts/auction-handler/FastLaneAuctionHandler.sol b/contracts/auction-handler/FastLaneAuctionHandler.sol index bfa9e71..e493c7c 100644 --- a/contracts/auction-handler/FastLaneAuctionHandler.sol +++ b/contracts/auction-handler/FastLaneAuctionHandler.sol @@ -378,6 +378,14 @@ contract FastLaneAuctionHandler is FastLaneAuctionHandlerEvents { emit RelayFeeCollected(_payor, block.coinbase, msg.value); } + function paySpecificValidatorFee(address _validator) external payable nonReentrant { + // TODO: block this when _validator == block.coinbase? + if (msg.value == 0) revert RelayValueIsZero(); + validatorsBalanceMap[_validator] += msg.value; + validatorsTotal += msg.value; + emit RelayFeeCollected(_validator, block.coinbase, msg.value); + } + // Part of the PaymentProcessor interface. In this case, this is used to transfer a validator's balances from one // PFL contract to a new deployment. function payValidator(address validator, uint256 startBlock, uint256 endBlock, uint256 totalAmount, bytes calldata) From 37da8f23e3e5262fd6cb19c391c5edc9558a15cb Mon Sep 17 00:00:00 2001 From: jj1980a Date: Tue, 6 Feb 2024 12:46:25 +0800 Subject: [PATCH 4/5] fix tests to accomodate new getValidatorIfNotCurrent function --- test/PFL_AuctionHandler.t.sol | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/test/PFL_AuctionHandler.t.sol b/test/PFL_AuctionHandler.t.sol index fee3b0e..e1a9feb 100644 --- a/test/PFL_AuctionHandler.t.sol +++ b/test/PFL_AuctionHandler.t.sol @@ -166,9 +166,9 @@ contract PFLAuctionHandlerTest is PFLHelper, FastLaneAuctionHandlerEvents, Test // Set the refund up vm.startPrank(VALIDATOR1); // should fail if validator is changing their own block - vm.expectRevert(FastLaneAuctionHandlerEvents.RelayImmutableBlockAuthorRate.selector); + vm.expectRevert(FastLaneAuctionHandlerEvents.RelayNotCurrentValidator.selector); PFR.updateValidatorRefundShare(0); - vm.coinbase(address(0)); + vm.coinbase(VALIDATOR2); PFR.updateValidatorRefundShare(5000); // 50% vm.coinbase(VALIDATOR1); vm.stopPrank(); @@ -365,6 +365,14 @@ contract PFLAuctionHandlerTest is PFLHelper, FastLaneAuctionHandlerEvents, Test vm.prank(SEARCHER_ADDRESS1, SEARCHER_ADDRESS1); PFR.submitFlashBid{value: bidAmount}(bidAmount, bytes32("randomTx"), address(SRE), searcherUnusedData); + // Validator can't collect fees when they are the current coinbase + vm.prank(VALIDATOR1); + vm.expectRevert(FastLaneAuctionHandlerEvents.RelayNotCurrentValidator.selector); + PFR.collectFees(); + + // Set another validator as coinbase for the rest of the test + vm.coinbase(VALIDATOR2); + uint256 snap = vm.snapshot(); // As V1 pay itself @@ -755,6 +763,7 @@ contract PFLAuctionHandlerTest is PFLHelper, FastLaneAuctionHandlerEvents, Test assertEq(PFR.getValidatorBlockOfLastWithdraw(VALIDATOR1), 0); // Returns block number of last withdraw + vm.coinbase(VALIDATOR2); vm.prank(VALIDATOR1); PFR.collectFees(); assertEq(PFR.getValidatorBlockOfLastWithdraw(VALIDATOR1), block.number); @@ -770,6 +779,14 @@ contract PFLAuctionHandlerTest is PFLHelper, FastLaneAuctionHandlerEvents, Test PFR.payValidatorFee{value: 1 ether}(USER); assertEq(PFR.getValidatorBalance(VALIDATOR1), 1 ether); + // Reverts if validator is the current coinbase + vm.prank(VALIDATOR1); + vm.expectRevert(FastLaneAuctionHandlerEvents.RelayNotCurrentValidator.selector); + PFR.collectFeesCustom(address(1), ""); + + // Set another validator as coinbase for the rest of the test + vm.coinbase(VALIDATOR2); + uint256 snap = vm.snapshot(); // Testing a working Payment Processor @@ -845,6 +862,7 @@ contract PFLAuctionHandlerTest is PFLHelper, FastLaneAuctionHandlerEvents, Test // Fast forward vm.warp(block.timestamp + 7 days); + vm.coinbase(VALIDATOR2); // This revert message comes from Solmate's SafeTransferLib, and is triggered by "REENTRANCY" revert // Use `forge test --match-test BlocksAllReentrancy -vvv` to see the inner revert message of "REENTRANCY" vm.expectRevert(bytes("ETH_TRANSFER_FAILED")); @@ -863,6 +881,7 @@ contract PFLAuctionHandlerTest is PFLHelper, FastLaneAuctionHandlerEvents, Test attackerPP1.setAttacker2(address(attackerPP2)); attackerPP1.setPayee(payee); + vm.coinbase(VALIDATOR2); vm.startPrank(VALIDATOR1); vm.expectRevert(FastLaneAuctionHandlerEvents.RelayUnapprovedReentrancy.selector); PFR.collectFeesCustom(address(attackerPP1), ""); From d81588d8e6d48558fecd8799f5ff35241ff200ba Mon Sep 17 00:00:00 2001 From: jj1980a Date: Tue, 6 Feb 2024 12:52:55 +0800 Subject: [PATCH 5/5] forward balance between PFL contracts test --- test/PFL_AuctionHandler.t.sol | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/PFL_AuctionHandler.t.sol b/test/PFL_AuctionHandler.t.sol index e1a9feb..1b75b13 100644 --- a/test/PFL_AuctionHandler.t.sol +++ b/test/PFL_AuctionHandler.t.sol @@ -888,6 +888,24 @@ contract PFLAuctionHandlerTest is PFLHelper, FastLaneAuctionHandlerEvents, Test vm.stopPrank(); } + function testForwardBalanceBetweenContracts() public { + uint256 startingBalance = 100; + FastLaneAuctionHandler PFRNew = new FastLaneAuctionHandler(); + + PFR.paySpecificValidatorFee{value: startingBalance}(VALIDATOR1); + PFRNew.paySpecificValidatorFee{value: startingBalance}(VALIDATOR1); + + assertEq(PFR.getValidatorBalance(VALIDATOR1), startingBalance); + assertEq(PFRNew.getValidatorBalance(VALIDATOR1), startingBalance); + + vm.coinbase(VALIDATOR2); + vm.prank(VALIDATOR1); + PFR.collectFeesCustom(address(PFRNew), ""); + + assertEq(PFR.getValidatorBalance(VALIDATOR1), 1); + assertEq(PFRNew.getValidatorBalance(VALIDATOR1), startingBalance * 2 - 1); + } + // Useful to get past the "validatorsBalanceMap[validator] > 0" checks function _donateOneWeiToValidatorBalance() internal { vm.prank(USER);