From f8c8e18357421dd5e1db86446a4e0e0f0e175047 Mon Sep 17 00:00:00 2001 From: Karl Bartel Date: Tue, 9 Apr 2024 16:36:42 +0200 Subject: [PATCH 1/3] Test fee currencies with failing debit/credit This should normally not happen, but we have to make sure that such a rare case won't crash geth nodes are cause stuck transactions that will get re-executed on every block. --- .gitignore | 1 + .../debug-fee-currency/DebugFeeCurrency.sol | 751 ++++++++++++++++++ .../debug-fee-currency/deploy_and_send_tx.sh | 13 + e2e_test/js-tests/send_tx.mjs | 37 + e2e_test/test_fee_currency_fails_on_credit.sh | 12 + e2e_test/test_fee_currency_fails_on_debit.sh | 10 + 6 files changed, 824 insertions(+) create mode 100644 e2e_test/debug-fee-currency/DebugFeeCurrency.sol create mode 100755 e2e_test/debug-fee-currency/deploy_and_send_tx.sh create mode 100755 e2e_test/js-tests/send_tx.mjs create mode 100755 e2e_test/test_fee_currency_fails_on_credit.sh create mode 100755 e2e_test/test_fee_currency_fails_on_debit.sh diff --git a/.gitignore b/.gitignore index f004cca458..3196c84848 100644 --- a/.gitignore +++ b/.gitignore @@ -51,3 +51,4 @@ profile.cov logs/ tests/spec-tests/ +e2e_test/debug-fee-currency/*.log diff --git a/e2e_test/debug-fee-currency/DebugFeeCurrency.sol b/e2e_test/debug-fee-currency/DebugFeeCurrency.sol new file mode 100644 index 0000000000..e5d6e1ce3a --- /dev/null +++ b/e2e_test/debug-fee-currency/DebugFeeCurrency.sol @@ -0,0 +1,751 @@ +pragma solidity ^0.8.13; + +// OpenZeppelin Contracts (last updated v5.0.0) (token/ERC20/ERC20.sol) + +// OpenZeppelin Contracts (last updated v5.0.0) (token/ERC20/IERC20.sol) + +/** + * @dev Interface of the ERC20 standard as defined in the EIP. + */ +interface IERC20 { + /** + * @dev Emitted when `value` tokens are moved from one account (`from`) to + * another (`to`). + * + * Note that `value` may be zero. + */ + event Transfer(address indexed from, address indexed to, uint256 value); + + /** + * @dev Emitted when the allowance of a `spender` for an `owner` is set by + * a call to {approve}. `value` is the new allowance. + */ + event Approval(address indexed owner, address indexed spender, uint256 value); + + /** + * @dev Returns the value of tokens in existence. + */ + function totalSupply() external view returns (uint256); + + /** + * @dev Returns the value of tokens owned by `account`. + */ + function balanceOf(address account) external view returns (uint256); + + /** + * @dev Moves a `value` amount of tokens from the caller's account to `to`. + * + * Returns a boolean value indicating whether the operation succeeded. + * + * Emits a {Transfer} event. + */ + function transfer(address to, uint256 value) external returns (bool); + + /** + * @dev Returns the remaining number of tokens that `spender` will be + * allowed to spend on behalf of `owner` through {transferFrom}. This is + * zero by default. + * + * This value changes when {approve} or {transferFrom} are called. + */ + function allowance(address owner, address spender) external view returns (uint256); + + /** + * @dev Sets a `value` amount of tokens as the allowance of `spender` over the + * caller's tokens. + * + * Returns a boolean value indicating whether the operation succeeded. + * + * IMPORTANT: Beware that changing an allowance with this method brings the risk + * that someone may use both the old and the new allowance by unfortunate + * transaction ordering. One possible solution to mitigate this race + * condition is to first reduce the spender's allowance to 0 and set the + * desired value afterwards: + * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729 + * + * Emits an {Approval} event. + */ + function approve(address spender, uint256 value) external returns (bool); + + /** + * @dev Moves a `value` amount of tokens from `from` to `to` using the + * allowance mechanism. `value` is then deducted from the caller's + * allowance. + * + * Returns a boolean value indicating whether the operation succeeded. + * + * Emits a {Transfer} event. + */ + function transferFrom(address from, address to, uint256 value) external returns (bool); +} + +// OpenZeppelin Contracts (last updated v5.0.0) (token/ERC20/extensions/IERC20Metadata.sol) + +/** + * @dev Interface for the optional metadata functions from the ERC20 standard. + */ +interface IERC20Metadata is IERC20 { + /** + * @dev Returns the name of the token. + */ + function name() external view returns (string memory); + + /** + * @dev Returns the symbol of the token. + */ + function symbol() external view returns (string memory); + + /** + * @dev Returns the decimals places of the token. + */ + function decimals() external view returns (uint8); +} + +// OpenZeppelin Contracts (last updated v5.0.1) (utils/Context.sol) + +/** + * @dev Provides information about the current execution context, including the + * sender of the transaction and its data. While these are generally available + * via msg.sender and msg.data, they should not be accessed in such a direct + * manner, since when dealing with meta-transactions the account sending and + * paying for execution may not be the actual sender (as far as an application + * is concerned). + * + * This contract is only required for intermediate, library-like contracts. + */ +abstract contract Context { + function _msgSender() internal view virtual returns (address) { + return msg.sender; + } + + function _msgData() internal view virtual returns (bytes calldata) { + return msg.data; + } + + function _contextSuffixLength() internal view virtual returns (uint256) { + return 0; + } +} + +// OpenZeppelin Contracts (last updated v5.0.0) (interfaces/draft-IERC6093.sol) + +/** + * @dev Standard ERC20 Errors + * Interface of the https://eips.ethereum.org/EIPS/eip-6093[ERC-6093] custom errors for ERC20 tokens. + */ +interface IERC20Errors { + /** + * @dev Indicates an error related to the current `balance` of a `sender`. Used in transfers. + * @param sender Address whose tokens are being transferred. + * @param balance Current balance for the interacting account. + * @param needed Minimum amount required to perform a transfer. + */ + error ERC20InsufficientBalance(address sender, uint256 balance, uint256 needed); + + /** + * @dev Indicates a failure with the token `sender`. Used in transfers. + * @param sender Address whose tokens are being transferred. + */ + error ERC20InvalidSender(address sender); + + /** + * @dev Indicates a failure with the token `receiver`. Used in transfers. + * @param receiver Address to which tokens are being transferred. + */ + error ERC20InvalidReceiver(address receiver); + + /** + * @dev Indicates a failure with the `spender`’s `allowance`. Used in transfers. + * @param spender Address that may be allowed to operate on tokens without being their owner. + * @param allowance Amount of tokens a `spender` is allowed to operate with. + * @param needed Minimum amount required to perform a transfer. + */ + error ERC20InsufficientAllowance(address spender, uint256 allowance, uint256 needed); + + /** + * @dev Indicates a failure with the `approver` of a token to be approved. Used in approvals. + * @param approver Address initiating an approval operation. + */ + error ERC20InvalidApprover(address approver); + + /** + * @dev Indicates a failure with the `spender` to be approved. Used in approvals. + * @param spender Address that may be allowed to operate on tokens without being their owner. + */ + error ERC20InvalidSpender(address spender); +} + +/** + * @dev Standard ERC721 Errors + * Interface of the https://eips.ethereum.org/EIPS/eip-6093[ERC-6093] custom errors for ERC721 tokens. + */ +interface IERC721Errors { + /** + * @dev Indicates that an address can't be an owner. For example, `address(0)` is a forbidden owner in EIP-20. + * Used in balance queries. + * @param owner Address of the current owner of a token. + */ + error ERC721InvalidOwner(address owner); + + /** + * @dev Indicates a `tokenId` whose `owner` is the zero address. + * @param tokenId Identifier number of a token. + */ + error ERC721NonexistentToken(uint256 tokenId); + + /** + * @dev Indicates an error related to the ownership over a particular token. Used in transfers. + * @param sender Address whose tokens are being transferred. + * @param tokenId Identifier number of a token. + * @param owner Address of the current owner of a token. + */ + error ERC721IncorrectOwner(address sender, uint256 tokenId, address owner); + + /** + * @dev Indicates a failure with the token `sender`. Used in transfers. + * @param sender Address whose tokens are being transferred. + */ + error ERC721InvalidSender(address sender); + + /** + * @dev Indicates a failure with the token `receiver`. Used in transfers. + * @param receiver Address to which tokens are being transferred. + */ + error ERC721InvalidReceiver(address receiver); + + /** + * @dev Indicates a failure with the `operator`’s approval. Used in transfers. + * @param operator Address that may be allowed to operate on tokens without being their owner. + * @param tokenId Identifier number of a token. + */ + error ERC721InsufficientApproval(address operator, uint256 tokenId); + + /** + * @dev Indicates a failure with the `approver` of a token to be approved. Used in approvals. + * @param approver Address initiating an approval operation. + */ + error ERC721InvalidApprover(address approver); + + /** + * @dev Indicates a failure with the `operator` to be approved. Used in approvals. + * @param operator Address that may be allowed to operate on tokens without being their owner. + */ + error ERC721InvalidOperator(address operator); +} + +/** + * @dev Standard ERC1155 Errors + * Interface of the https://eips.ethereum.org/EIPS/eip-6093[ERC-6093] custom errors for ERC1155 tokens. + */ +interface IERC1155Errors { + /** + * @dev Indicates an error related to the current `balance` of a `sender`. Used in transfers. + * @param sender Address whose tokens are being transferred. + * @param balance Current balance for the interacting account. + * @param needed Minimum amount required to perform a transfer. + * @param tokenId Identifier number of a token. + */ + error ERC1155InsufficientBalance(address sender, uint256 balance, uint256 needed, uint256 tokenId); + + /** + * @dev Indicates a failure with the token `sender`. Used in transfers. + * @param sender Address whose tokens are being transferred. + */ + error ERC1155InvalidSender(address sender); + + /** + * @dev Indicates a failure with the token `receiver`. Used in transfers. + * @param receiver Address to which tokens are being transferred. + */ + error ERC1155InvalidReceiver(address receiver); + + /** + * @dev Indicates a failure with the `operator`’s approval. Used in transfers. + * @param operator Address that may be allowed to operate on tokens without being their owner. + * @param owner Address of the current owner of a token. + */ + error ERC1155MissingApprovalForAll(address operator, address owner); + + /** + * @dev Indicates a failure with the `approver` of a token to be approved. Used in approvals. + * @param approver Address initiating an approval operation. + */ + error ERC1155InvalidApprover(address approver); + + /** + * @dev Indicates a failure with the `operator` to be approved. Used in approvals. + * @param operator Address that may be allowed to operate on tokens without being their owner. + */ + error ERC1155InvalidOperator(address operator); + + /** + * @dev Indicates an array length mismatch between ids and values in a safeBatchTransferFrom operation. + * Used in batch transfers. + * @param idsLength Length of the array of token identifiers + * @param valuesLength Length of the array of token amounts + */ + error ERC1155InvalidArrayLength(uint256 idsLength, uint256 valuesLength); +} + +/** + * @dev Implementation of the {IERC20} interface. + * + * This implementation is agnostic to the way tokens are created. This means + * that a supply mechanism has to be added in a derived contract using {_mint}. + * + * TIP: For a detailed writeup see our guide + * https://forum.openzeppelin.com/t/how-to-implement-erc20-supply-mechanisms/226[How + * to implement supply mechanisms]. + * + * The default value of {decimals} is 18. To change this, you should override + * this function so it returns a different value. + * + * We have followed general OpenZeppelin Contracts guidelines: functions revert + * instead returning `false` on failure. This behavior is nonetheless + * conventional and does not conflict with the expectations of ERC20 + * applications. + * + * Additionally, an {Approval} event is emitted on calls to {transferFrom}. + * This allows applications to reconstruct the allowance for all accounts just + * by listening to said events. Other implementations of the EIP may not emit + * these events, as it isn't required by the specification. + */ +abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors { + mapping(address account => uint256) private _balances; + + mapping(address account => mapping(address spender => uint256)) private _allowances; + + uint256 private _totalSupply; + + string private _name; + string private _symbol; + + /** + * @dev Sets the values for {name} and {symbol}. + * + * All two of these values are immutable: they can only be set once during + * construction. + */ + constructor(string memory name_, string memory symbol_) { + _name = name_; + _symbol = symbol_; + } + + /** + * @dev Returns the name of the token. + */ + function name() public view virtual returns (string memory) { + return _name; + } + + /** + * @dev Returns the symbol of the token, usually a shorter version of the + * name. + */ + function symbol() public view virtual returns (string memory) { + return _symbol; + } + + /** + * @dev Returns the number of decimals used to get its user representation. + * For example, if `decimals` equals `2`, a balance of `505` tokens should + * be displayed to a user as `5.05` (`505 / 10 ** 2`). + * + * Tokens usually opt for a value of 18, imitating the relationship between + * Ether and Wei. This is the default value returned by this function, unless + * it's overridden. + * + * NOTE: This information is only used for _display_ purposes: it in + * no way affects any of the arithmetic of the contract, including + * {IERC20-balanceOf} and {IERC20-transfer}. + */ + function decimals() public view virtual returns (uint8) { + return 18; + } + + /** + * @dev See {IERC20-totalSupply}. + */ + function totalSupply() public view virtual returns (uint256) { + return _totalSupply; + } + + /** + * @dev See {IERC20-balanceOf}. + */ + function balanceOf(address account) public view virtual returns (uint256) { + return _balances[account]; + } + + /** + * @dev See {IERC20-transfer}. + * + * Requirements: + * + * - `to` cannot be the zero address. + * - the caller must have a balance of at least `value`. + */ + function transfer(address to, uint256 value) public virtual returns (bool) { + address owner = _msgSender(); + _transfer(owner, to, value); + return true; + } + + /** + * @dev See {IERC20-allowance}. + */ + function allowance(address owner, address spender) public view virtual returns (uint256) { + return _allowances[owner][spender]; + } + + /** + * @dev See {IERC20-approve}. + * + * NOTE: If `value` is the maximum `uint256`, the allowance is not updated on + * `transferFrom`. This is semantically equivalent to an infinite approval. + * + * Requirements: + * + * - `spender` cannot be the zero address. + */ + function approve(address spender, uint256 value) public virtual returns (bool) { + address owner = _msgSender(); + _approve(owner, spender, value); + return true; + } + + /** + * @dev See {IERC20-transferFrom}. + * + * Emits an {Approval} event indicating the updated allowance. This is not + * required by the EIP. See the note at the beginning of {ERC20}. + * + * NOTE: Does not update the allowance if the current allowance + * is the maximum `uint256`. + * + * Requirements: + * + * - `from` and `to` cannot be the zero address. + * - `from` must have a balance of at least `value`. + * - the caller must have allowance for ``from``'s tokens of at least + * `value`. + */ + function transferFrom(address from, address to, uint256 value) public virtual returns (bool) { + address spender = _msgSender(); + _spendAllowance(from, spender, value); + _transfer(from, to, value); + return true; + } + + /** + * @dev Moves a `value` amount of tokens from `from` to `to`. + * + * This internal function is equivalent to {transfer}, and can be used to + * e.g. implement automatic token fees, slashing mechanisms, etc. + * + * Emits a {Transfer} event. + * + * NOTE: This function is not virtual, {_update} should be overridden instead. + */ + function _transfer(address from, address to, uint256 value) internal { + if (from == address(0)) { + revert ERC20InvalidSender(address(0)); + } + if (to == address(0)) { + revert ERC20InvalidReceiver(address(0)); + } + _update(from, to, value); + } + + /** + * @dev Transfers a `value` amount of tokens from `from` to `to`, or alternatively mints (or burns) if `from` + * (or `to`) is the zero address. All customizations to transfers, mints, and burns should be done by overriding + * this function. + * + * Emits a {Transfer} event. + */ + function _update(address from, address to, uint256 value) internal virtual { + if (from == address(0)) { + // Overflow check required: The rest of the code assumes that totalSupply never overflows + _totalSupply += value; + } else { + uint256 fromBalance = _balances[from]; + if (fromBalance < value) { + revert ERC20InsufficientBalance(from, fromBalance, value); + } + unchecked { + // Overflow not possible: value <= fromBalance <= totalSupply. + _balances[from] = fromBalance - value; + } + } + + if (to == address(0)) { + unchecked { + // Overflow not possible: value <= totalSupply or value <= fromBalance <= totalSupply. + _totalSupply -= value; + } + } else { + unchecked { + // Overflow not possible: balance + value is at most totalSupply, which we know fits into a uint256. + _balances[to] += value; + } + } + + emit Transfer(from, to, value); + } + + /** + * @dev Creates a `value` amount of tokens and assigns them to `account`, by transferring it from address(0). + * Relies on the `_update` mechanism + * + * Emits a {Transfer} event with `from` set to the zero address. + * + * NOTE: This function is not virtual, {_update} should be overridden instead. + */ + function _mint(address account, uint256 value) internal { + if (account == address(0)) { + revert ERC20InvalidReceiver(address(0)); + } + _update(address(0), account, value); + } + + /** + * @dev Destroys a `value` amount of tokens from `account`, lowering the total supply. + * Relies on the `_update` mechanism. + * + * Emits a {Transfer} event with `to` set to the zero address. + * + * NOTE: This function is not virtual, {_update} should be overridden instead + */ + function _burn(address account, uint256 value) internal { + if (account == address(0)) { + revert ERC20InvalidSender(address(0)); + } + _update(account, address(0), value); + } + + /** + * @dev Sets `value` as the allowance of `spender` over the `owner` s tokens. + * + * This internal function is equivalent to `approve`, and can be used to + * e.g. set automatic allowances for certain subsystems, etc. + * + * Emits an {Approval} event. + * + * Requirements: + * + * - `owner` cannot be the zero address. + * - `spender` cannot be the zero address. + * + * Overrides to this logic should be done to the variant with an additional `bool emitEvent` argument. + */ + function _approve(address owner, address spender, uint256 value) internal { + _approve(owner, spender, value, true); + } + + /** + * @dev Variant of {_approve} with an optional flag to enable or disable the {Approval} event. + * + * By default (when calling {_approve}) the flag is set to true. On the other hand, approval changes made by + * `_spendAllowance` during the `transferFrom` operation set the flag to false. This saves gas by not emitting any + * `Approval` event during `transferFrom` operations. + * + * Anyone who wishes to continue emitting `Approval` events on the`transferFrom` operation can force the flag to + * true using the following override: + * ``` + * function _approve(address owner, address spender, uint256 value, bool) internal virtual override { + * super._approve(owner, spender, value, true); + * } + * ``` + * + * Requirements are the same as {_approve}. + */ + function _approve(address owner, address spender, uint256 value, bool emitEvent) internal virtual { + if (owner == address(0)) { + revert ERC20InvalidApprover(address(0)); + } + if (spender == address(0)) { + revert ERC20InvalidSpender(address(0)); + } + _allowances[owner][spender] = value; + if (emitEvent) { + emit Approval(owner, spender, value); + } + } + + /** + * @dev Updates `owner` s allowance for `spender` based on spent `value`. + * + * Does not update the allowance value in case of infinite allowance. + * Revert if not enough allowance is available. + * + * Does not emit an {Approval} event. + */ + function _spendAllowance(address owner, address spender, uint256 value) internal virtual { + uint256 currentAllowance = allowance(owner, spender); + if (currentAllowance != type(uint256).max) { + if (currentAllowance < value) { + revert ERC20InsufficientAllowance(spender, currentAllowance, value); + } + unchecked { + _approve(owner, spender, currentAllowance - value, false); + } + } + } +} + +/** + * @notice This interface should be implemented for tokens which are supposed to + * act as fee currencies on the Celo blockchain, meaning that they can be + * used to pay gas fees for CIP-64 transactions (and some older tx types). + * See https://github.com/celo-org/celo-proposals/blob/master/CIPs/cip-0064.md + * + * @notice Before executing a tx with non-empty `feeCurrency` field, the fee + * currency's `debitGasFees` function is called to reserve the maximum + * amount of gas token that tx can spend. After the tx has been executed, the + * `creditGasFees` function is called to refund any unused gas and credit + * the spent fees to the appropriate recipients. Events which are emitted in + * these functions will show up for every tx using the token as a + * fee currency. + * + * @dev Requirements: + * - The functions will be called by the blockchain client with `msg.sender + * == address(0)`. If this condition is not met, the functions must + * revert to prevent malicious users from crediting their accounts directly. + * - `creditGasFees` must credit all specified amounts. If this is not + * possible the functions must revert to prevent inconsistencies between + * the debited and credited amounts. + * + * @dev Notes on compatibility: + * - There are two versions of `creditGasFees`: one for the current + * (2024-01-16) blockchain implementation and a more future-proof version + * that omits deprecated fields and accommodates potential new recipients + * that might become necessary on later blockchain implementations. Both + * versions should be implemented to increase compatibility. + */ +interface IFeeCurrency is IERC20 { + /// @notice Called before transaction execution to reserve the maximum amount of gas + /// that can be used by the transaction. + /// - The implementation must deduct `value` from `from`'s balance. + /// - Must revert if `msg.sender` is not the zero address. + function debitGasFees(address from, uint256 value) external; + + /// @notice New function signature, will be used when all fee currencies have migrated. + /// Credited amounts include gas refund, base fee and tip. Future additions + /// may include L1 gas fee when Celo becomes and L2. + /// - The implementation must increase each `recipient`'s balance by corresponding `amount`. + /// - Must revert if `msg.sender` is not the zero address. + /// - Must revert if `recipients` and `amounts` have different lengths. + function creditGasFees(address[] calldata recipients, uint256[] calldata amounts) external; + + /// @notice Old function signature for backwards compatibility + /// - Must revert if `msg.sender` is not the zero address. + /// - `refundAmount` must be credited to `refundRecipient` + /// - `tipAmount` must be credited to `tipRecipient` + /// - `baseFeeAmount` must be credited to `baseFeeRecipient` + /// - `_gatewayFeeRecipient` and `_gatewayFeeAmount` only exist for backwards + /// compatibility reasons and will always be zero. + function creditGasFees( + address refundRecipient, + address tipRecipient, + address _gatewayFeeRecipient, + address baseFeeRecipient, + uint256 refundAmount, + uint256 tipAmount, + uint256 _gatewayFeeAmount, + uint256 baseFeeAmount + ) external; +} + +contract FeeCurrency is ERC20, IFeeCurrency { + constructor(uint256 initialSupply) ERC20("ExampleFeeCurrency", "EFC") { + _mint(msg.sender, initialSupply); + } + + modifier onlyVm() { + require(msg.sender == address(0), "Only VM can call"); + _; + } + + function debitGasFees(address from, uint256 value) external onlyVm { + _burn(from, value); + } + + // New function signature, will be used when all fee currencies have migrated + function creditGasFees(address[] calldata recipients, uint256[] calldata amounts) public onlyVm { + require(recipients.length == amounts.length, "Recipients and amounts must be the same length."); + + for (uint256 i = 0; i < recipients.length; i++) { + _mint(recipients[i], amounts[i]); + } + } + + // Old function signature for backwards compatibility + function creditGasFees( + address from, + address feeRecipient, + address, // gatewayFeeRecipient, unused + address communityFund, + uint256 refund, + uint256 tipTxFee, + uint256, // gatewayFee, unused + uint256 baseTxFee + ) public onlyVm { + // Calling the new creditGasFees would make sense here, but that is not + // possible due to its calldata arguments. + _mint(from, refund); + _mint(feeRecipient, tipTxFee); + _mint(communityFund, baseTxFee); + } +} + +contract DebugFeeCurrency is ERC20, IFeeCurrency { + bool failOnDebit; + bool failOnCredit; + + constructor(uint256 initialSupply, bool _failOnDebit, bool _failOnCredit) ERC20("DebugFeeCurrency", "DFC") { + _mint(msg.sender, initialSupply); + failOnDebit = _failOnDebit; + failOnCredit = _failOnCredit; + } + + modifier onlyVm() { + require(msg.sender == address(0), "Only VM can call"); + _; + } + + function debitGasFees(address from, uint256 value) external onlyVm { + require(!failOnDebit, "This DebugFeeCurrency always fails in debitGasFees!"); + _burn(from, value); + } + + // New function signature, will be used when all fee currencies have migrated + function creditGasFees(address[] calldata recipients, uint256[] calldata amounts) public onlyVm { + require(!failOnCredit, "This DebugFeeCurrency always fails in (new) creditGasFees!"); + require(recipients.length == amounts.length, "Recipients and amounts must be the same length."); + + for (uint256 i = 0; i < recipients.length; i++) { + _mint(recipients[i], amounts[i]); + } + } + + // Old function signature for backwards compatibility + function creditGasFees( + address from, + address feeRecipient, + address, // gatewayFeeRecipient, unused + address communityFund, + uint256 refund, + uint256 tipTxFee, + uint256, // gatewayFee, unused + uint256 baseTxFee + ) public onlyVm { + require(!failOnCredit, "This DebugFeeCurrency always fails in (old) creditGasFees!"); + // Calling the new creditGasFees would make sense here, but that is not + // possible due to its calldata arguments. + _mint(from, refund); + _mint(feeRecipient, tipTxFee); + _mint(communityFund, baseTxFee); + } +} + diff --git a/e2e_test/debug-fee-currency/deploy_and_send_tx.sh b/e2e_test/debug-fee-currency/deploy_and_send_tx.sh new file mode 100755 index 0000000000..151910adb2 --- /dev/null +++ b/e2e_test/debug-fee-currency/deploy_and_send_tx.sh @@ -0,0 +1,13 @@ +#!/bin/bash +#shellcheck disable=SC2034,SC2155,SC2086 +set -xeo pipefail + +export FEE_CURRENCY=$(\ + forge create --root . --contracts . --private-key $ACC_PRIVKEY DebugFeeCurrency.sol:DebugFeeCurrency --constructor-args '100000000000000000000000000' $1 $2 --json\ + | jq .deployedTo -r) + +cast send --private-key $ACC_PRIVKEY $FEE_CURRENCY_WHITELIST_ADDR 'addToken(address)' $FEE_CURRENCY --legacy +cast send --private-key $ACC_PRIVKEY $SORTED_ORACLES_ADDR 'setMedianRate(address, uint256)' $FEE_CURRENCY 2000000000000000000000000 --legacy +echo Fee currency: $FEE_CURRENCY + +(cd ../js-tests/ && ./send_tx.mjs "$(cast chain-id)" $ACC_PRIVKEY $FEE_CURRENCY) diff --git a/e2e_test/js-tests/send_tx.mjs b/e2e_test/js-tests/send_tx.mjs new file mode 100755 index 0000000000..892bb1fcfe --- /dev/null +++ b/e2e_test/js-tests/send_tx.mjs @@ -0,0 +1,37 @@ +#!/usr/bin/env node +import { createPublicClient, createWalletClient, http, defineChain } from 'viem' +import { celoAlfajores } from 'viem/chains' +import { privateKeyToAccount } from 'viem/accounts' + +const [chainId, privateKey, feeCurrency] = process.argv.slice(2) +const devChain = defineChain({ + ...celoAlfajores, + id: parseInt(chainId, 10), + name: 'local dev chain', + network: 'dev', + rpcUrls: { + default: { + http: ['http://127.0.0.1:8545'], + }, + }, +}) + +const account = privateKeyToAccount(privateKey) +const walletClient = createWalletClient({ + account, + chain: devChain, + transport: http(), +}) + +const request = await walletClient.prepareTransactionRequest({ + account, + to: '0x00000000000000000000000000000000DeaDBeef', + value: 2, + gas: 90000, + feeCurrency, + maxFeePerGas: 2000000000n, + maxPriorityFeePerGas: 0n, +}) +const signature = await walletClient.signTransaction(request) +const hash = await walletClient.sendRawTransaction({ serializedTransaction: signature }) +console.log(hash) diff --git a/e2e_test/test_fee_currency_fails_on_credit.sh b/e2e_test/test_fee_currency_fails_on_credit.sh new file mode 100755 index 0000000000..137ea6c227 --- /dev/null +++ b/e2e_test/test_fee_currency_fails_on_credit.sh @@ -0,0 +1,12 @@ +#!/bin/bash +#shellcheck disable=SC2086 +set -eo pipefail + +source shared.sh + +# Expect that the creditGasFees failed and is logged by geth +tail -f -n0 geth.log > debug-fee-currency/geth.partial.log & # start log capture +(cd debug-fee-currency && ./deploy_and_send_tx.sh false true) +sleep 0.1 +kill %1 # stop log capture +grep "This DebugFeeCurrency always fails in (old) creditGasFees!" debug-fee-currency/geth.partial.log diff --git a/e2e_test/test_fee_currency_fails_on_debit.sh b/e2e_test/test_fee_currency_fails_on_debit.sh new file mode 100755 index 0000000000..0b4e4438f3 --- /dev/null +++ b/e2e_test/test_fee_currency_fails_on_debit.sh @@ -0,0 +1,10 @@ +#!/bin/bash +#shellcheck disable=SC2086 +set -eo pipefail + +source shared.sh + +# Expect that the debitGasFees fails during tx submission +(cd debug-fee-currency && ./deploy_and_send_tx.sh true false) &> debug-fee-currency/send_tx.log || true +grep "debitGasFees reverted: This DebugFeeCurrency always fails in debitGasFees!" debug-fee-currency/send_tx.log \ + || (cat debug-fee-currency/send_tx.log && false) From 216816cfca2a9cc31dd7e18ce043ccfd7b5afd67 Mon Sep 17 00:00:00 2001 From: Karl Bartel Date: Tue, 9 Apr 2024 16:40:41 +0200 Subject: [PATCH 2/3] Add err detail on credit revert --- core/state_transition.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/state_transition.go b/core/state_transition.go index 7da774e756..1b5cd5f2bf 100644 --- a/core/state_transition.go +++ b/core/state_transition.go @@ -728,7 +728,7 @@ func (st *StateTransition) distributeTxFees() error { l1Cost, _ = exchange.ConvertGoldToCurrency(st.evm.Context.ExchangeRates, feeCurrency, l1Cost) } if err := contracts.CreditFees(st.evm, feeCurrency, from, st.evm.Context.Coinbase, feeHandlerAddress, params.OptimismL1FeeRecipient, refund, tipTxFee, baseTxFee, l1Cost); err != nil { - log.Error("Error crediting", "from", from, "coinbase", st.evm.Context.Coinbase, "feeHandler", feeHandlerAddress) + log.Error("Error crediting", "from", from, "coinbase", st.evm.Context.Coinbase, "feeHandler", feeHandlerAddress, "err", err) return err } } From d98df3f024d52ac1d78fded0004a8be7261bc78c Mon Sep 17 00:00:00 2001 From: Karl Bartel Date: Tue, 9 Apr 2024 16:41:19 +0200 Subject: [PATCH 3/3] Fix panic: clear access lists before debiting gas This is important because debiting gas for fee currencies changes the access lists. If the lists are cleared afterwards, this causes an inconsistency between the access list and the journal. When reverting to a previous snapshot, this inconsistency causes a panic. This would happen if creditGasFees reverts. --- core/state_transition.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/core/state_transition.go b/core/state_transition.go index 1b5cd5f2bf..46d5c4e025 100644 --- a/core/state_transition.go +++ b/core/state_transition.go @@ -537,6 +537,18 @@ func (st *StateTransition) innerTransitionDb() (*ExecutionResult, error) { // 5. there is no overflow when calculating intrinsic gas // 6. caller has enough balance to cover asset transfer for **topmost** call + var ( + msg = st.msg + sender = vm.AccountRef(msg.From) + rules = st.evm.ChainConfig().Rules(st.evm.Context.BlockNumber, st.evm.Context.Random != nil, st.evm.Context.Time) + contractCreation = msg.To == nil + ) + + // Execute the preparatory steps for state transition which includes: + // - prepare accessList(post-berlin) + // - reset transient storage(eip 1153) + st.state.Prepare(rules, msg.From, st.evm.Context.Coinbase, msg.To, vm.ActivePrecompiles(rules), msg.AccessList) + // Check clauses 1-3, buy gas if everything is correct if err := st.preCheck(); err != nil { return nil, err @@ -549,13 +561,6 @@ func (st *StateTransition) innerTransitionDb() (*ExecutionResult, error) { }() } - var ( - msg = st.msg - sender = vm.AccountRef(msg.From) - rules = st.evm.ChainConfig().Rules(st.evm.Context.BlockNumber, st.evm.Context.Random != nil, st.evm.Context.Time) - contractCreation = msg.To == nil - ) - // Check clauses 4-5, subtract intrinsic gas if everything is correct gas, err := IntrinsicGas(msg.Data, msg.AccessList, contractCreation, rules.IsHomestead, rules.IsIstanbul, rules.IsShanghai, msg.FeeCurrency) if err != nil { @@ -576,11 +581,6 @@ func (st *StateTransition) innerTransitionDb() (*ExecutionResult, error) { return nil, fmt.Errorf("%w: code size %v limit %v", ErrMaxInitCodeSizeExceeded, len(msg.Data), params.MaxInitCodeSize) } - // Execute the preparatory steps for state transition which includes: - // - prepare accessList(post-berlin) - // - reset transient storage(eip 1153) - st.state.Prepare(rules, msg.From, st.evm.Context.Coinbase, msg.To, vm.ActivePrecompiles(rules), msg.AccessList) - var ( ret []byte vmerr error // vm errors do not effect consensus and are therefore not assigned to err