diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 05564b8352f..1fbe8e99f1c 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -653,7 +653,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 * in a governance proposal to recover tokens or Ether that was sent to the governor contract by mistake. * Note that if the executor is simply the governor itself, use of `relay` is redundant. */ - function relay(address target, uint256 value, bytes calldata data) external payable virtual onlyGovernance { + function relay(address target, uint256 value, bytes calldata data) public payable virtual onlyGovernance { (bool success, bytes memory returndata) = target.call{value: value}(data); Address.verifyCallResult(success, returndata); } diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 17fef9215b1..f92446af3bc 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -26,7 +26,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { bytes32 public constant PROPOSER_ROLE = keccak256("PROPOSER_ROLE"); bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE"); bytes32 public constant CANCELLER_ROLE = keccak256("CANCELLER_ROLE"); - uint256 internal constant _DONE_TIMESTAMP = uint256(1); + uint256 internal constant DONE_TIMESTAMP = uint256(1); mapping(bytes32 id => uint256) private _timestamps; uint256 private _minDelay; @@ -207,7 +207,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { uint256 timestamp = getTimestamp(id); if (timestamp == 0) { return OperationState.Unset; - } else if (timestamp == _DONE_TIMESTAMP) { + } else if (timestamp == DONE_TIMESTAMP) { return OperationState.Done; } else if (timestamp > block.timestamp) { return OperationState.Waiting; @@ -432,7 +432,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { if (!isOperationReady(id)) { revert TimelockUnexpectedOperationState(id, _encodeStateBitmap(OperationState.Ready)); } - _timestamps[id] = _DONE_TIMESTAMP; + _timestamps[id] = DONE_TIMESTAMP; } /** @@ -445,7 +445,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { * - the caller must be the timelock itself. This can only be achieved by scheduling and later executing * an operation where the timelock is the target and the data is the ABI-encoded call to this function. */ - function updateDelay(uint256 newDelay) external virtual { + function updateDelay(uint256 newDelay) public virtual { address sender = _msgSender(); if (sender != address(this)) { revert TimelockUnauthorizedCaller(sender); diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol index dce13f1b3f9..601267e2925 100644 --- a/contracts/governance/extensions/GovernorTimelockCompound.sol +++ b/contracts/governance/extensions/GovernorTimelockCompound.sol @@ -136,7 +136,7 @@ abstract contract GovernorTimelockCompound is Governor { /** * @dev Accept admin right over the timelock. */ - // solhint-disable-next-line private-vars-leading-underscore + // solhint-disable-next-line openzeppelin/leading-underscore function __acceptAdmin() public { _timelock.acceptAdmin(); } @@ -154,7 +154,7 @@ abstract contract GovernorTimelockCompound is Governor { * CAUTION: It is not recommended to change the timelock while there are other queued governance proposals. */ - function updateTimelock(ICompoundTimelock newTimelock) external virtual onlyGovernance { + function updateTimelock(ICompoundTimelock newTimelock) public virtual onlyGovernance { _updateTimelock(newTimelock); } diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index b3f3b26c9ea..edce4e4fbf2 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -146,7 +146,7 @@ abstract contract GovernorTimelockControl is Governor { * * CAUTION: It is not recommended to change the timelock while there are other queued governance proposals. */ - function updateTimelock(TimelockController newTimelock) external virtual onlyGovernance { + function updateTimelock(TimelockController newTimelock) public virtual onlyGovernance { _updateTimelock(newTimelock); } diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index 2f6034df932..9ba224ce2cb 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -73,7 +73,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { * - Must be called through a governance proposal. * - New numerator must be smaller or equal to the denominator. */ - function updateQuorumNumerator(uint256 newQuorumNumerator) external virtual onlyGovernance { + function updateQuorumNumerator(uint256 newQuorumNumerator) public virtual onlyGovernance { _updateQuorumNumerator(newQuorumNumerator); } diff --git a/contracts/metatx/ERC2771Forwarder.sol b/contracts/metatx/ERC2771Forwarder.sol index 23f4099947b..3ba53222557 100644 --- a/contracts/metatx/ERC2771Forwarder.sol +++ b/contracts/metatx/ERC2771Forwarder.sol @@ -61,7 +61,7 @@ contract ERC2771Forwarder is EIP712, Nonces { bytes signature; } - bytes32 internal constant _FORWARD_REQUEST_TYPEHASH = + bytes32 internal constant FORWARD_REQUEST_TYPEHASH = keccak256( "ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)" ); @@ -222,7 +222,7 @@ contract ERC2771Forwarder is EIP712, Nonces { (address recovered, ECDSA.RecoverError err, ) = _hashTypedDataV4( keccak256( abi.encode( - _FORWARD_REQUEST_TYPEHASH, + FORWARD_REQUEST_TYPEHASH, request.from, request.to, request.value, diff --git a/contracts/mocks/proxy/UUPSUpgradeableMock.sol b/contracts/mocks/proxy/UUPSUpgradeableMock.sol index 8c5641e6ca3..b44c2be606d 100644 --- a/contracts/mocks/proxy/UUPSUpgradeableMock.sol +++ b/contracts/mocks/proxy/UUPSUpgradeableMock.sol @@ -28,8 +28,8 @@ contract UUPSUpgradeableUnsafeMock is UUPSUpgradeableMock { } } -contract UUPSUnsupportedProxiableUUID is UUPSUpgradeableMock { - function proxiableUUID() external pure override returns (bytes32) { +contract UUPSUnsupportedProxiableUUIDMock is NonUpgradeableMock { + function proxiableUUID() external pure returns (bytes32) { return keccak256("invalid UUID"); } } diff --git a/contracts/proxy/utils/UUPSUpgradeable.sol b/contracts/proxy/utils/UUPSUpgradeable.sol index 6be78251d96..2a88b021156 100644 --- a/contracts/proxy/utils/UUPSUpgradeable.sol +++ b/contracts/proxy/utils/UUPSUpgradeable.sol @@ -71,7 +71,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable { * bricking a proxy that upgrades to it, by delegating to itself until out of gas. Thus it is critical that this * function revert if invoked through a proxy. This is guaranteed by the `notDelegated` modifier. */ - function proxiableUUID() external view virtual notDelegated returns (bytes32) { + function proxiableUUID() external view notDelegated returns (bytes32) { return ERC1967Utils.IMPLEMENTATION_SLOT; } diff --git a/contracts/token/ERC20/extensions/ERC20Permit.sol b/contracts/token/ERC20/extensions/ERC20Permit.sol index 3b181640030..b89df28be9e 100644 --- a/contracts/token/ERC20/extensions/ERC20Permit.sol +++ b/contracts/token/ERC20/extensions/ERC20Permit.sol @@ -71,7 +71,7 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712, Nonces { /// @inheritdoc IERC20Permit // solhint-disable-next-line func-name-mixedcase - function DOMAIN_SEPARATOR() external view virtual returns (bytes32) { + function DOMAIN_SEPARATOR() external view returns (bytes32) { return _domainSeparatorV4(); } } diff --git a/contracts/utils/Multicall.sol b/contracts/utils/Multicall.sol index 94222feb0f4..c986c3c7cde 100644 --- a/contracts/utils/Multicall.sol +++ b/contracts/utils/Multicall.sol @@ -23,7 +23,7 @@ abstract contract Multicall is Context { * @dev Receives and executes a batch of function calls on this contract. * @custom:oz-upgrades-unsafe-allow-reachable delegatecall */ - function multicall(bytes[] calldata data) external virtual returns (bytes[] memory results) { + function multicall(bytes[] calldata data) public virtual returns (bytes[] memory results) { bytes memory context = msg.sender == _msgSender() ? new bytes(0) : msg.data[msg.data.length - _contextSuffixLength():]; diff --git a/package-lock.json b/package-lock.json index 8e6df71f0d1..ac6c165a06b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -43,7 +43,7 @@ "prettier-plugin-solidity": "^2.0.0", "rimraf": "^6.0.0", "semver": "^7.3.5", - "solhint": "^6.0.0", + "solhint": "^6.0.1", "solhint-plugin-openzeppelin": "file:scripts/solhint-custom", "solidity-ast": "^0.4.50", "solidity-coverage": "^0.8.14", @@ -9408,13 +9408,13 @@ } }, "node_modules/solhint": { - "version": "6.0.0", - "resolved": "https://registry.npmjs.org/solhint/-/solhint-6.0.0.tgz", - "integrity": "sha512-PQGfwFqfeYdebi2tEG1fhVfMjqSzbW3Noz+LYf8UusKe5nkikCghdgEjYQPcGfFZj4snlVyJQt//AaxkubOtVQ==", + "version": "6.0.1", + "resolved": "https://registry.npmjs.org/solhint/-/solhint-6.0.1.tgz", + "integrity": "sha512-Lew5nhmkXqHPybzBzkMzvvWkpOJSSLTkfTZwRriWvfR2naS4YW2PsjVGaoX9tZFmHh7SuS+e2GEGo5FPYYmJ8g==", "dev": true, "license": "MIT", "dependencies": { - "@solidity-parser/parser": "^0.20.0", + "@solidity-parser/parser": "^0.20.2", "ajv": "^6.12.6", "ajv-errors": "^1.0.1", "antlr4": "^4.13.1-patch-1", @@ -9424,7 +9424,6 @@ "commander": "^10.0.0", "cosmiconfig": "^8.0.0", "fast-diff": "^1.2.0", - "fs-extra": "^11.1.0", "glob": "^8.0.3", "ignore": "^5.2.4", "js-yaml": "^4.1.0", @@ -9432,7 +9431,6 @@ "lodash": "^4.17.21", "pluralize": "^8.0.0", "semver": "^7.5.2", - "strip-ansi": "^6.0.1", "table": "^6.8.1", "text-table": "^0.2.0" }, @@ -9448,9 +9446,9 @@ "link": true }, "node_modules/solhint/node_modules/@solidity-parser/parser": { - "version": "0.20.1", - "resolved": "https://registry.npmjs.org/@solidity-parser/parser/-/parser-0.20.1.tgz", - "integrity": "sha512-58I2sRpzaQUN+jJmWbHfbWf9AKfzqCI8JAdFB0vbyY+u8tBRcuTt9LxzasvR0LGQpcRv97eyV7l61FQ3Ib7zVw==", + "version": "0.20.2", + "resolved": "https://registry.npmjs.org/@solidity-parser/parser/-/parser-0.20.2.tgz", + "integrity": "sha512-rbu0bzwNvMcwAjH86hiEAcOeRI2EeK8zCkHDrFykh/Al8mvJeFmjy3UrE7GYQjNwOgbGUUtCn5/k8CB8zIu7QA==", "dev": true, "license": "MIT" }, @@ -9505,21 +9503,6 @@ "node": ">=14" } }, - "node_modules/solhint/node_modules/fs-extra": { - "version": "11.3.0", - "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-11.3.0.tgz", - "integrity": "sha512-Z4XaCL6dUDHfP/jT25jJKMmtxvuwbkrD1vNSMFlo9lNLY2c5FHYSQgHPRZUjAB26TpDEoW9HCOgplrdbaPV/ew==", - "dev": true, - "license": "MIT", - "dependencies": { - "graceful-fs": "^4.2.0", - "jsonfile": "^6.0.1", - "universalify": "^2.0.0" - }, - "engines": { - "node": ">=14.14" - } - }, "node_modules/solhint/node_modules/glob": { "version": "8.1.0", "resolved": "https://registry.npmjs.org/glob/-/glob-8.1.0.tgz", @@ -9561,19 +9544,6 @@ "dev": true, "license": "MIT" }, - "node_modules/solhint/node_modules/jsonfile": { - "version": "6.1.0", - "resolved": "https://registry.npmjs.org/jsonfile/-/jsonfile-6.1.0.tgz", - "integrity": "sha512-5dgndWOriYSm5cnYaJNhalLNDKOqFwyDB/rr1E9ZsGciGvKPs8R2xYGCacuf3z6K1YKDz182fd+fY3cn3pMqXQ==", - "dev": true, - "license": "MIT", - "dependencies": { - "universalify": "^2.0.0" - }, - "optionalDependencies": { - "graceful-fs": "^4.1.6" - } - }, "node_modules/solhint/node_modules/minimatch": { "version": "5.1.6", "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-5.1.6.tgz", @@ -9604,16 +9574,6 @@ "url": "https://github.com/prettier/prettier?sponsor=1" } }, - "node_modules/solhint/node_modules/universalify": { - "version": "2.0.1", - "resolved": "https://registry.npmjs.org/universalify/-/universalify-2.0.1.tgz", - "integrity": "sha512-gptHNQghINnc/vTGIk0SOFGFNXw7JVrlRUtConJRlvaw6DuX0wO5Jeko9sWrMBhh+PsYAZ7oXAiOnf/UKogyiw==", - "dev": true, - "license": "MIT", - "engines": { - "node": ">= 10.0.0" - } - }, "node_modules/solidity-ast": { "version": "0.4.60", "resolved": "https://registry.npmjs.org/solidity-ast/-/solidity-ast-0.4.60.tgz", diff --git a/package.json b/package.json index f6960972ad5..c01775bd963 100644 --- a/package.json +++ b/package.json @@ -86,7 +86,7 @@ "prettier-plugin-solidity": "^2.0.0", "rimraf": "^6.0.0", "semver": "^7.3.5", - "solhint": "^6.0.0", + "solhint": "^6.0.1", "solhint-plugin-openzeppelin": "file:scripts/solhint-custom", "solidity-ast": "^0.4.50", "solidity-coverage": "^0.8.14", diff --git a/scripts/solhint-custom/index.js b/scripts/solhint-custom/index.js index 9625027eefe..9788f72e9e3 100644 --- a/scripts/solhint-custom/index.js +++ b/scripts/solhint-custom/index.js @@ -1,6 +1,9 @@ const path = require('path'); const minimatch = require('minimatch'); +const { isFallbackFunction } = require('solhint/lib/common/ast-types'); +const { hasLeadingUnderscore } = require('solhint/lib/common/identifier-naming'); + // Files matching these patterns will be ignored unless a rule has `static global = true` const ignore = ['contracts/mocks/**/*', 'test/**/*']; @@ -14,31 +17,24 @@ class Base { } } - error(node, message) { - if (!this.ignored) { + require(condition, node, message) { + if (!condition && !this.ignored) { this.reporter.error(node, this.ruleId, message); } } } module.exports = [ - class extends Base { - static ruleId = 'interface-names'; - - ContractDefinition(node) { - if (node.kind === 'interface' && !/^I[A-Z]/.test(node.name)) { - this.error(node, 'Interface names should have a capital I prefix'); - } - } - }, - class extends Base { static ruleId = 'private-variables'; VariableDeclaration(node) { - const constantOrImmutable = node.isDeclaredConst || node.isImmutable; - if (node.isStateVar && !constantOrImmutable && node.visibility !== 'private') { - this.error(node, 'State variables must be private'); + if (node.isStateVar) { + this.require( + node.isDeclaredConst || node.isImmutable || node.visibility === 'private', + node, + 'State variables must be private', + ); } } }, @@ -48,37 +44,75 @@ module.exports = [ VariableDeclaration(node) { if (node.isDeclaredConst) { - // TODO: expand visibility and fix - if (node.visibility === 'private' && /^_/.test(node.name)) { - this.error(node, 'Constant variables should not have leading underscore'); + this.require(!hasLeadingUnderscore(node.name), node, 'Constant variables should not have leading underscore'); + } else if (node.isStateVar) { + switch (node.visibility) { + case 'private': + this.require(hasLeadingUnderscore(node.name), node, 'Private state variables must have leading underscore'); + break; + case 'internal': + this.require( + hasLeadingUnderscore(node.name), + node, + 'Internal state variables must have leading underscore', + ); + break; + case 'public': + this.require( + !hasLeadingUnderscore(node.name), + node, + 'Public state variables should not have leading underscore', + ); + break; } - } else if (node.visibility === 'private' && !/^_/.test(node.name)) { - this.error(node, 'Non-constant private variables must have leading underscore'); } } FunctionDefinition(node) { - if (node.visibility === 'private' || (node.visibility === 'internal' && node.parent.kind !== 'library')) { - if (!/^_/.test(node.name)) { - this.error(node, 'Private and internal functions must have leading underscore'); - } + switch (node.visibility) { + case 'external': + this.require(!hasLeadingUnderscore(node.name), node, 'External functions should not have leading underscore'); + break; + case 'public': + this.require(!hasLeadingUnderscore(node.name), node, 'Public functions should not have leading underscore'); + break; + case 'internal': + this.require( + hasLeadingUnderscore(node.name) !== (node.parent.kind === 'library'), + node, + node.parent.kind === 'library' + ? 'Library internal functions should not have leading underscore' + : 'Non-library internal functions must have leading underscore', + ); + break; + case 'private': + this.require(hasLeadingUnderscore(node.name), node, 'Private functions must have leading underscore'); + break; } - if (node.visibility === 'internal' && node.parent.kind === 'library') { - if (/^_/.test(node.name)) { - this.error(node, 'Library internal functions should not have leading underscore'); - } + } + }, + + class extends Base { + static ruleId = 'no-external-virtual'; + + FunctionDefinition(node) { + if (node.visibility == 'external' && node.isVirtual) { + this.require(isFallbackFunction(node), node, 'Functions should not be external and virtual'); } } }, - // TODO: re-enable and fix - // class extends Base { - // static ruleId = 'no-external-virtual'; - // - // FunctionDefinition(node) { - // if (node.visibility == 'external' && node.isVirtual) { - // this.error(node, 'Functions should not be external and virtual'); - // } - // } - // }, + class extends Base { + static ruleId = 'no-public-library'; + + FunctionDefinition(node) { + if (node.parent.kind === 'library') { + this.require( + node.visibility === 'internal' || node.visibility === 'private', + node, + 'Library functions should be internal or private', + ); + } + } + }, ]; diff --git a/test/metatx/ERC2771Forwarder.t.sol b/test/metatx/ERC2771Forwarder.t.sol index 605ae62097e..224367377ff 100644 --- a/test/metatx/ERC2771Forwarder.t.sol +++ b/test/metatx/ERC2771Forwarder.t.sol @@ -27,7 +27,7 @@ contract ERC2771ForwarderMock is ERC2771Forwarder { _hashTypedDataV4( keccak256( abi.encode( - _FORWARD_REQUEST_TYPEHASH, + FORWARD_REQUEST_TYPEHASH, request.from, request.to, request.value, diff --git a/test/proxy/utils/UUPSUpgradeable.test.js b/test/proxy/utils/UUPSUpgradeable.test.js index 17f86576168..476d1e67663 100644 --- a/test/proxy/utils/UUPSUpgradeable.test.js +++ b/test/proxy/utils/UUPSUpgradeable.test.js @@ -9,7 +9,7 @@ async function fixture() { const implUpgradeOk = await ethers.deployContract('UUPSUpgradeableMock'); const implUpgradeUnsafe = await ethers.deployContract('UUPSUpgradeableUnsafeMock'); const implUpgradeNonUUPS = await ethers.deployContract('NonUpgradeableMock'); - const implUnsupportedUUID = await ethers.deployContract('UUPSUnsupportedProxiableUUID'); + const implUnsupportedUUID = await ethers.deployContract('UUPSUnsupportedProxiableUUIDMock'); // Used for testing non ERC1967 compliant proxies (clones are proxies that don't use the ERC1967 implementation slot) const cloneFactory = await ethers.deployContract('$Clones');