From e6df717d8d59c24f211b43d391b49d75bf395c20 Mon Sep 17 00:00:00 2001 From: cgewecke Date: Sun, 25 Feb 2024 15:12:40 -0800 Subject: [PATCH] Fix duplicate hash logic (#868) --- lib/collector.js | 10 +- test/integration/standard.js | 5 + .../modifiers/contracts/ModifiersD.sol | 12 +++ .../projects/modifiers/external/Context.sol | 28 +++++ .../projects/modifiers/external/Ownable.sol | 100 ++++++++++++++++++ .../projects/modifiers/test/modifiers.js | 12 +++ 6 files changed, 165 insertions(+), 2 deletions(-) create mode 100644 test/sources/projects/modifiers/contracts/ModifiersD.sol create mode 100644 test/sources/projects/modifiers/external/Context.sol create mode 100644 test/sources/projects/modifiers/external/Ownable.sol diff --git a/lib/collector.js b/lib/collector.js index 062de100..320d2f36 100644 --- a/lib/collector.js +++ b/lib/collector.js @@ -28,6 +28,8 @@ class DataCollector { this.lastHash = null; this.viaIR = viaIR; + this.pcZeroCounter = 0; + this.lastPcZeroCount = 0; } /** @@ -36,11 +38,13 @@ class DataCollector { * @param {Object} info vm step info */ step(info){ + if (info.pc === 0) this.pcZeroCounter++; + try { if (this.validOpcodes[info.opcode.name] && info.stack.length > 0){ const idx = info.stack.length - 1; let hash = '0x' + info.stack[idx].toString(16); - this._registerHash(hash) + this._registerHash(hash); } } catch (err) { /*Ignore*/ }; } @@ -66,8 +70,10 @@ class DataCollector { if(this.instrumentationData[hash]){ // abi.encode (used to circumvent viaIR) sometimes puts the hash on the stack twice - if (this.lastHash !== hash) { + // We should only skip duplicate hashes *within* a transaction (see issue #863) + if (this.lastHash !== hash || this.lastPcZeroCount !== this.pcZeroCounter) { this.lastHash = hash; + this.lastPcZeroCount = this.pcZeroCounter; this.instrumentationData[hash].hits++ } return; diff --git a/test/integration/standard.js b/test/integration/standard.js index a52a89fd..3c5a8ba4 100644 --- a/test/integration/standard.js +++ b/test/integration/standard.js @@ -424,6 +424,11 @@ describe('Hardhat Plugin: standard use cases', function() { file: mock.pathToContract(hardhatConfig, 'ModifiersC.sol'), pct: 25 }, + { + file: mock.pathToContract(hardhatConfig, 'ModifiersD.sol'), + pct: 100 + }, + ]; verify.branchCoverage(expected); diff --git a/test/sources/projects/modifiers/contracts/ModifiersD.sol b/test/sources/projects/modifiers/contracts/ModifiersD.sol new file mode 100644 index 00000000..54ad6472 --- /dev/null +++ b/test/sources/projects/modifiers/contracts/ModifiersD.sol @@ -0,0 +1,12 @@ +pragma solidity >=0.8.0 <0.9.0; + +import "./../external/Ownable.sol"; + +contract ModifiersD is Ownable { + constructor() Ownable(msg.sender) {} + + function a() public onlyOwner { + } +} + + diff --git a/test/sources/projects/modifiers/external/Context.sol b/test/sources/projects/modifiers/external/Context.sol new file mode 100644 index 00000000..89e54441 --- /dev/null +++ b/test/sources/projects/modifiers/external/Context.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v5.0.1) (utils/Context.sol) + +pragma solidity >=0.8.0 <0.9.0; + +/** + * @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; + } +} diff --git a/test/sources/projects/modifiers/external/Ownable.sol b/test/sources/projects/modifiers/external/Ownable.sol new file mode 100644 index 00000000..a333867a --- /dev/null +++ b/test/sources/projects/modifiers/external/Ownable.sol @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v5.0.0) (access/Ownable.sol) + +pragma solidity >=0.8.0 <0.9.0; + +import {Context} from "./Context.sol"; + +/** + * @dev Contract module which provides a basic access control mechanism, where + * there is an account (an owner) that can be granted exclusive access to + * specific functions. + * + * The initial owner is set to the address provided by the deployer. This can + * later be changed with {transferOwnership}. + * + * This module is used through inheritance. It will make available the modifier + * `onlyOwner`, which can be applied to your functions to restrict their use to + * the owner. + */ +contract Ownable is Context { + address private _owner; + + /** + * @dev The caller account is not authorized to perform an operation. + */ + error OwnableUnauthorizedAccount(address account); + + /** + * @dev The owner is not a valid owner account. (eg. `address(0)`) + */ + error OwnableInvalidOwner(address owner); + + event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); + + /** + * @dev Initializes the contract setting the address provided by the deployer as the initial owner. + */ + constructor(address initialOwner) { + if (initialOwner == address(0)) { + revert OwnableInvalidOwner(address(0)); + } + _transferOwnership(initialOwner); + } + + /** + * @dev Throws if called by any account other than the owner. + */ + modifier onlyOwner() { + _checkOwner(); + _; + } + + /** + * @dev Returns the address of the current owner. + */ + function owner() public view returns (address) { + return _owner; + } + + /** + * @dev Throws if the sender is not the owner. + */ + function _checkOwner() internal view { + if (owner() != _msgSender()) { + revert OwnableUnauthorizedAccount(_msgSender()); + } + } + + /** + * @dev Leaves the contract without owner. It will not be possible to call + * `onlyOwner` functions. Can only be called by the current owner. + * + * NOTE: Renouncing ownership will leave the contract without an owner, + * thereby disabling any functionality that is only available to the owner. + */ + function renounceOwnership() public onlyOwner { + _transferOwnership(address(0)); + } + + /** + * @dev Transfers ownership of the contract to a new account (`newOwner`). + * Can only be called by the current owner. + */ + function transferOwnership(address newOwner) public onlyOwner { + if (newOwner == address(0)) { + revert OwnableInvalidOwner(address(0)); + } + _transferOwnership(newOwner); + } + + /** + * @dev Transfers ownership of the contract to a new account (`newOwner`). + * Internal function without access restriction. + */ + function _transferOwnership(address newOwner) internal { + address oldOwner = _owner; + _owner = newOwner; + emit OwnershipTransferred(oldOwner, newOwner); + } +} diff --git a/test/sources/projects/modifiers/test/modifiers.js b/test/sources/projects/modifiers/test/modifiers.js index 8aa1c043..42df22c3 100644 --- a/test/sources/projects/modifiers/test/modifiers.js +++ b/test/sources/projects/modifiers/test/modifiers.js @@ -1,5 +1,6 @@ const ModifiersA = artifacts.require("ModifiersA"); const ModifiersC = artifacts.require("ModifiersC"); +const ModifiersD = artifacts.require("ModifiersD"); contract("Modifiers", function(accounts) { let instance; @@ -7,6 +8,7 @@ contract("Modifiers", function(accounts) { before(async () => { A = await ModifiersA.new(); C = await ModifiersC.new(); + D = await ModifiersD.new(); }) it('simpleSet (overridden method)', async function(){ @@ -37,4 +39,14 @@ contract("Modifiers", function(accounts) { /* ignore */ } }); + + it('when false & true branches are hit in immediate succesion by EOA (issue #863)', async function(){ + try { + await D.a({from: accounts[1]}); + } catch (e) { + /* ignore */ + } + + await D.a({from: accounts[0]}); + }) });