From 96d066d3afe222c8f7632240c9a75b431fd4b902 Mon Sep 17 00:00:00 2001 From: Ryan Sauge Date: Fri, 1 Sep 2023 11:51:33 +0200 Subject: [PATCH 1/5] Remove function kill + add function deactivateContract in PauseModule --- contracts/libraries/Errors.sol | 6 +- contracts/modules/CMTAT_BASE.sol | 2 +- .../security/OnlyDelegateCallModule.sol | 28 --- .../mandatory => security}/PauseModule.sol | 25 +- .../modules/wrapper/mandatory/BaseModule.sol | 18 +- .../wrapper/optional/ValidationModule.sol | 2 +- .../CMTATSnapshot/CMTAT_BASE_SnapshotTest.sol | 2 +- contracts/test/killTest/BaseModuleTest.sol | 109 --------- contracts/test/killTest/CMTATKillTest.sol | 226 ------------------ package.json | 8 +- test/common/BaseModuleCommon.js | 29 --- test/common/PauseModuleCommon.js | 55 ++++- test/proxy/general/KillImplementation.test.js | 45 ---- test/proxy/general/Proxy.test.js | 20 +- 14 files changed, 88 insertions(+), 487 deletions(-) delete mode 100644 contracts/modules/security/OnlyDelegateCallModule.sol rename contracts/modules/{wrapper/mandatory => security}/PauseModule.sol (68%) delete mode 100644 contracts/test/killTest/BaseModuleTest.sol delete mode 100644 contracts/test/killTest/CMTATKillTest.sol delete mode 100644 test/proxy/general/KillImplementation.test.js diff --git a/contracts/libraries/Errors.sol b/contracts/libraries/Errors.sol index 3f3eaad9..cd7801a8 100644 --- a/contracts/libraries/Errors.sol +++ b/contracts/libraries/Errors.sol @@ -28,9 +28,6 @@ library Errors { error CMTAT_SnapshotModule_NoSnapshotScheduled(); error CMTAT_SnapshotModule_SnapshotNotFound(); - // OnlyDelegateCallModule - error CMTAT_OnlyDelegateCallModule_DirectCallToImplementation(); - // ERC20BaseModule error CMTAT_ERC20BaseModule_WrongAllowance( address spender, @@ -61,4 +58,7 @@ library Errors { // AuthorizationModule error CMTAT_AuthorizationModule_AddressZeroNotAllowed(); + + // PauseModule + error CMTAT_PauseModule_ContractIsDeactivated(); } diff --git a/contracts/modules/CMTAT_BASE.sol b/contracts/modules/CMTAT_BASE.sol index be48f710..069f5233 100644 --- a/contracts/modules/CMTAT_BASE.sol +++ b/contracts/modules/CMTAT_BASE.sol @@ -16,12 +16,12 @@ SnapshotModule: Add this import in case you add the SnapshotModule import "./wrapper/optional/SnapshotModule.sol"; */ -import "./wrapper/mandatory/PauseModule.sol"; import "./wrapper/optional/ValidationModule.sol"; import "./wrapper/optional/MetaTxModule.sol"; import "./wrapper/optional/DebtModule/DebtBaseModule.sol"; import "./wrapper/optional/DebtModule/CreditEventsModule.sol"; import "./security/AuthorizationModule.sol"; +import "./security/PauseModule.sol"; import "../interfaces/IEIP1404/IEIP1404Wrapper.sol"; import "../libraries/Errors.sol"; diff --git a/contracts/modules/security/OnlyDelegateCallModule.sol b/contracts/modules/security/OnlyDelegateCallModule.sol deleted file mode 100644 index a7e61b92..00000000 --- a/contracts/modules/security/OnlyDelegateCallModule.sol +++ /dev/null @@ -1,28 +0,0 @@ -//SPDX-License-Identifier: MPL-2.0 - -pragma solidity ^0.8.20; - -import "../../libraries/Errors.sol"; - -/** -@dev When a contract is deployed with a proxy, insure that some functions (e.g. delegatecall and selfdestruct) can only be triggered through proxies -and not on the implementation contract itself. -*/ -abstract contract OnlyDelegateCallModule { - /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment - address private immutable self = address(this); - - function checkDelegateCall() private view { - if (address(this) == self) { - revert Errors - .CMTAT_OnlyDelegateCallModule_DirectCallToImplementation(); - } - } - - modifier onlyDelegateCall(bool deployedWithProxy) { - if (deployedWithProxy) { - checkDelegateCall(); - } - _; - } -} diff --git a/contracts/modules/wrapper/mandatory/PauseModule.sol b/contracts/modules/security/PauseModule.sol similarity index 68% rename from contracts/modules/wrapper/mandatory/PauseModule.sol rename to contracts/modules/security/PauseModule.sol index bdac355d..0a14ab51 100644 --- a/contracts/modules/wrapper/mandatory/PauseModule.sol +++ b/contracts/modules/security/PauseModule.sol @@ -2,9 +2,9 @@ pragma solidity ^0.8.20; -import "../../../../openzeppelin-contracts-upgradeable/contracts/security/PausableUpgradeable.sol"; -import "../../../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol"; -import "../../security/AuthorizationModule.sol"; +import "../../../openzeppelin-contracts-upgradeable/contracts/security/PausableUpgradeable.sol"; +import "../../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol"; +import "./AuthorizationModule.sol"; /** * @dev ERC20 token with pausable token transfers, minting and burning. @@ -16,6 +16,8 @@ import "../../security/AuthorizationModule.sol"; abstract contract PauseModule is PausableUpgradeable, AuthorizationModule { string internal constant TEXT_TRANSFER_REJECTED_PAUSED = "All transfers paused"; + bool isDeactivated; + event Deactivated(address account); function __PauseModule_init(address admin) internal onlyInitializing { /* OpenZeppelin */ @@ -61,8 +63,25 @@ abstract contract PauseModule is PausableUpgradeable, AuthorizationModule { * - the caller must have the `PAUSER_ROLE`. */ function unpause() public onlyRole(PAUSER_ROLE) { + if(isDeactivated){ + revert Errors.CMTAT_PauseModule_ContractIsDeactivated(); + } _unpause(); } + /** + @notice destroys the contract and send the remaining ethers in the contract to the sender + Warning: the operation is irreversible, be careful + */ + /// @custom:oz-upgrades-unsafe-allow selfdestruct + function deactivateContract() + public + onlyRole(DEFAULT_ADMIN_ROLE) + { + isDeactivated = true; + _pause(); + emit Deactivated(_msgSender()); + } + uint256[50] private __gap; } diff --git a/contracts/modules/wrapper/mandatory/BaseModule.sol b/contracts/modules/wrapper/mandatory/BaseModule.sol index 434b1402..9db94add 100644 --- a/contracts/modules/wrapper/mandatory/BaseModule.sol +++ b/contracts/modules/wrapper/mandatory/BaseModule.sol @@ -5,11 +5,9 @@ pragma solidity ^0.8.20; // required OZ imports here import "../../../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol"; import "../../security/AuthorizationModule.sol"; -import "../../security/OnlyDelegateCallModule.sol"; - import "../../../libraries/Errors.sol"; -abstract contract BaseModule is AuthorizationModule, OnlyDelegateCallModule { +abstract contract BaseModule is AuthorizationModule { // to initialize inside the implementation constructor when deployed with a Proxy bool internal deployedWithProxy; /* Events */ @@ -27,6 +25,7 @@ abstract contract BaseModule is AuthorizationModule, OnlyDelegateCallModule { string public information; uint256 public flag; + /* Initializers */ /** * @dev Sets the values for {name} and {symbol}. @@ -110,18 +109,5 @@ abstract contract BaseModule is AuthorizationModule, OnlyDelegateCallModule { emit Flag(flag_); } - /** - @notice destroys the contract and send the remaining ethers in the contract to the sender - Warning: the operation is irreversible, be careful - */ - /// @custom:oz-upgrades-unsafe-allow selfdestruct - function kill() - public - onlyRole(DEFAULT_ADMIN_ROLE) - onlyDelegateCall(deployedWithProxy) - { - selfdestruct(payable(_msgSender())); - } - uint256[50] private __gap; } diff --git a/contracts/modules/wrapper/optional/ValidationModule.sol b/contracts/modules/wrapper/optional/ValidationModule.sol index cb643585..8143a1c1 100644 --- a/contracts/modules/wrapper/optional/ValidationModule.sol +++ b/contracts/modules/wrapper/optional/ValidationModule.sol @@ -5,7 +5,7 @@ pragma solidity ^0.8.20; import "../../../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol"; import "../../security/AuthorizationModule.sol"; import "../../internal/ValidationModuleInternal.sol"; -import "../mandatory/PauseModule.sol"; +import "../../security/PauseModule.sol"; import "../mandatory/EnforcementModule.sol"; import "../../../libraries/Errors.sol"; diff --git a/contracts/test/CMTATSnapshot/CMTAT_BASE_SnapshotTest.sol b/contracts/test/CMTATSnapshot/CMTAT_BASE_SnapshotTest.sol index 82606835..cbd40408 100644 --- a/contracts/test/CMTATSnapshot/CMTAT_BASE_SnapshotTest.sol +++ b/contracts/test/CMTATSnapshot/CMTAT_BASE_SnapshotTest.sol @@ -16,12 +16,12 @@ SnapshotModule: Add this import in case you add the SnapshotModule */ import "../../modules/wrapper/optional/SnapshotModule.sol"; -import "../../modules/wrapper/mandatory/PauseModule.sol"; import "../../modules/wrapper/optional/ValidationModule.sol"; import "../../modules/wrapper/optional/MetaTxModule.sol"; import "../../modules/wrapper/optional/DebtModule/DebtBaseModule.sol"; import "../../modules/wrapper/optional/DebtModule/CreditEventsModule.sol"; import "../../modules/security/AuthorizationModule.sol"; +import "../../modules/security/PauseModule.sol"; import "../../interfaces/IEIP1404/IEIP1404Wrapper.sol"; import "../../libraries/Errors.sol"; diff --git a/contracts/test/killTest/BaseModuleTest.sol b/contracts/test/killTest/BaseModuleTest.sol deleted file mode 100644 index 0b06bfa8..00000000 --- a/contracts/test/killTest/BaseModuleTest.sol +++ /dev/null @@ -1,109 +0,0 @@ -//SPDX-License-Identifier: MPL-2.0 - -pragma solidity ^0.8.20; - -// required OZ imports here -import "../../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol"; -import "../../modules/security/AuthorizationModule.sol"; -import "../../modules/security/OnlyDelegateCallModule.sol"; - -/** -@title A BaseModule version only for TESTING -@dev This version has removed the check of access control on the kill function -The only remaining protection is the call to the modifier onlyDelegateCall -*/ -abstract contract BaseModuleTest is - Initializable, - AuthorizationModule, - OnlyDelegateCallModule -{ - // @dev we removed the access control to check onlyDelegateCall - /// @custom:oz-upgrades-unsafe-allow selfdestruct - function kill() public onlyDelegateCall(deployedWithProxy) { - selfdestruct(payable(_msgSender())); - } - - //******* Code from BaseModule, not modified *******/ - - bool internal deployedWithProxy; - /* Events */ - event TermSet(string indexed newTerm); - event TokenIdSet(string indexed newTokenId); - event InformationSet(string indexed newInformation); - event FlagSet(uint256 indexed newFlag); - - /* Variables */ - string public tokenId; - string public terms; - string public information; - uint256 public flag; - - /* Initializers */ - /** - * @dev Sets the values for {name} and {symbol}. - * - * All two of these values are immutable: they can only be set once during - * construction. - */ - function __Base_init( - string memory tokenId_, - string memory terms_, - string memory information_, - uint256 flag_, - address admin - ) internal onlyInitializing { - /* OpenZeppelin */ - __Context_init_unchained(); - // AccessControlUpgradeable inherits from ERC165Upgradeable - __ERC165_init_unchained(); - // AuthorizationModule inherits from AccessControlUpgradeable - __AccessControl_init_unchained(); - - /* Wrapper */ - __AuthorizationModule_init_unchained(admin); - - /* own function */ - __Base_init_unchained(tokenId_, terms_, information_, flag_); - } - - function __Base_init_unchained( - string memory tokenId_, - string memory terms_, - string memory information_, - uint256 flag_ - ) internal onlyInitializing { - tokenId = tokenId_; - terms = terms_; - information = information_; - flag = flag_; - } - - /* Methods */ - function setTokenId( - string calldata tokenId_ - ) public onlyRole(DEFAULT_ADMIN_ROLE) { - tokenId = tokenId_; - emit TokenIdSet(tokenId_); - } - - function setTerms( - string calldata terms_ - ) public onlyRole(DEFAULT_ADMIN_ROLE) { - terms = terms_; - emit TermSet(terms_); - } - - function setInformation( - string calldata information_ - ) public onlyRole(DEFAULT_ADMIN_ROLE) { - information = information_; - emit InformationSet(information_); - } - - function setFlag(uint256 flag_) public onlyRole(DEFAULT_ADMIN_ROLE) { - flag = flag_; - emit FlagSet(flag_); - } - - uint256[50] private __gap; -} diff --git a/contracts/test/killTest/CMTATKillTest.sol b/contracts/test/killTest/CMTATKillTest.sol deleted file mode 100644 index 3ec654cb..00000000 --- a/contracts/test/killTest/CMTATKillTest.sol +++ /dev/null @@ -1,226 +0,0 @@ -//SPDX-License-Identifier: MPL-2.0 - -pragma solidity ^0.8.20; - -// required OZ imports here -import "../../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initializable.sol"; -import "../../../openzeppelin-contracts-upgradeable/contracts/utils/ContextUpgradeable.sol"; -import "./BaseModuleTest.sol"; -import "../../modules/wrapper/mandatory/BurnModule.sol"; -import "../../modules/wrapper/mandatory/MintModule.sol"; -import "../../modules/wrapper/mandatory/BurnModule.sol"; -import "../../modules/wrapper/mandatory/EnforcementModule.sol"; -import "../../modules/wrapper/mandatory/ERC20BaseModule.sol"; -import "../../modules/wrapper/mandatory/PauseModule.sol"; -// import "../../modules/wrapper/optional/SnapshotModule.sol"; -import "../../modules/wrapper/optional/ValidationModule.sol"; -import "../../modules/wrapper/optional/MetaTxModule.sol"; -import "../../modules/wrapper/optional/DebtModule/DebtBaseModule.sol"; -import "../../modules/wrapper/optional/DebtModule/CreditEventsModule.sol"; -import "../../modules/security/AuthorizationModule.sol"; -import "../../modules/security/OnlyDelegateCallModule.sol"; -import "../../interfaces/IEIP1404/IEIP1404Wrapper.sol"; - -import "../../libraries/Errors.sol"; - -/** -@title A CMTAT version only for TESTING -@dev This version inherits from BaseModuleTest instead of BaseModule -*/ -contract CMTAT_KILL_TEST is - Initializable, - ContextUpgradeable, - BaseModuleTest, - PauseModule, - MintModule, - BurnModule, - EnforcementModule, - ValidationModule, - MetaTxModule, - //SnapshotModule, - ERC20BaseModule, - DebtBaseModule, - CreditEventsModule -{ - // CMTAT_PROXY constructor - /** - @notice Contract version for the deployment with a proxy - @param forwarderIrrevocable address of the forwarder, required for the gasless support - */ - /// @custom:oz-upgrades-unsafe-allow constructor - constructor( - address forwarderIrrevocable - ) MetaTxModule(forwarderIrrevocable) { - // Initialize the variable for the implementation - deployedWithProxy = true; - // Disable the possibility to initialize the implementation - _disableInitializers(); - } - - /** - @notice - initialize the proxy contract - The calls to this function will revert if the contract was deployed without a proxy - */ - function initialize( - address admin, - string memory nameIrrevocable, - string memory symbolIrrevocable, - uint8 decimalsIrrevocable, - string memory tokenId_, - string memory terms_, - IEIP1404Wrapper ruleEngine_, - string memory information_, - uint256 flag_ - ) public initializer { - __CMTAT_init( - admin, - nameIrrevocable, - symbolIrrevocable, - decimalsIrrevocable, - tokenId_, - terms_, - ruleEngine_, - information_, - flag_ - ); - } - - /** - @dev calls the different initialize functions from the different modules - */ - function __CMTAT_init( - address admin, - string memory nameIrrevocable, - string memory symbolIrrevocable, - uint8 decimalsIrrevocable, - string memory tokenId_, - string memory terms_, - IEIP1404Wrapper ruleEngine_, - string memory information_, - uint256 flag_ - ) internal onlyInitializing { - /* OpenZeppelin library */ - // OZ init_unchained functions are called firstly due to inheritance - __Context_init_unchained(); - __ERC20_init_unchained(nameIrrevocable, symbolIrrevocable); - // AccessControlUpgradeable inherits from ERC165Upgradeable - __ERC165_init_unchained(); - // AuthorizationModule inherits from AccessControlUpgradeable - __AccessControl_init_unchained(); - __Pausable_init_unchained(); - - /* Internal Modules */ - __Enforcement_init_unchained(); - /* - SnapshotModule: - Add this call in case you add the SnapshotModule - __Snapshot_init_unchained(); - */ - __Validation_init_unchained(ruleEngine_); - - /* Wrapper */ - // AuthorizationModule_init_unchained is called firstly due to inheritance - __AuthorizationModule_init_unchained(admin); - __BurnModule_init_unchained(); - __MintModule_init_unchained(); - // EnforcementModule_init_unchained is called before ValidationModule_init_unchained due to inheritance - __EnforcementModule_init_unchained(); - __ERC20Module_init_unchained(decimalsIrrevocable); - // PauseModule_init_unchained is called before ValidationModule_init_unchained due to inheritance - __PauseModule_init_unchained(); - __ValidationModule_init_unchained(); - - /* - SnapshotModule: - Add this call in case you add the SnapshotModule - __SnasphotModule_init_unchained(); - */ - - /* Other modules */ - __DebtBaseModule_init_unchained(); - __CreditEvents_init_unchained(); - __Base_init_unchained(tokenId_, terms_, information_, flag_); - - /* own function */ - __CMTAT_init_unchained(); - } - - function __CMTAT_init_unchained() internal onlyInitializing { - // no variable to initialize - } - - /** - @notice Returns the number of decimals used to get its user representation. - */ - function decimals() - public - view - virtual - override(ERC20Upgradeable, ERC20BaseModule) - returns (uint8) - { - return ERC20BaseModule.decimals(); - } - - function transferFrom( - address sender, - address recipient, - uint256 amount - ) - public - virtual - override(ERC20Upgradeable, ERC20BaseModule) - returns (bool) - { - return ERC20BaseModule.transferFrom(sender, recipient, amount); - } - - /* - @dev - SnapshotModule: - - override SnapshotModuleInternal if you add the SnapshotModule - e.g. override(SnapshotModuleInternal, ERC20Upgradeable) - - remove the keyword view - */ - function _update( - address from, - address to, - uint256 amount - ) internal view override(ERC20Upgradeable) { - if (!ValidationModule.validateTransfer(from, to, amount)) - revert Errors.CMTAT_InvalidTransfer(from, to, amount); - // We call the SnapshotModule only if the transfer is valid - /* - SnapshotModule: - Add this call in case you add the SnapshotModule - SnapshotModuleInternal._update(from, to, amount); - */ - } - - /** - @dev This surcharge is not necessary if you do not use the MetaTxModule - */ - function _msgSender() - internal - view - override(MetaTxModule, ContextUpgradeable) - returns (address sender) - { - return MetaTxModule._msgSender(); - } - - /** - @dev This surcharge is not necessary if you do not use the MetaTxModule - */ - function _msgData() - internal - view - override(MetaTxModule, ContextUpgradeable) - returns (bytes calldata) - { - return MetaTxModule._msgData(); - } - - uint256[50] private __gap; -} diff --git a/package.json b/package.json index f0e36002..e29ca180 100644 --- a/package.json +++ b/package.json @@ -23,9 +23,9 @@ "uml": "npx sol2uml class contracts -c", "uml-i-eip1404": "npx sol2uml class -c contracts/interfaces/IEIP1404", "uml-partial": "npx sol2uml class contracts -c -b CMTAT_STANDALONE && npx sol2uml class contracts -c -b CMTAT_PROXY && npx sol2uml class contracts -c -b CMTAT_BASE", - "uml-modules-mandatory": "npx sol2uml class contracts -c -b BaseModule && npx sol2uml class contracts -c -b BurnModule && npx sol2uml class contracts -c -b EnforcementModule && npx sol2uml class contracts -c -b ERC20BaseModule && npx sol2uml class contracts -c -b MintModule && npx sol2uml class contracts -c -b PauseModule", + "uml-modules-mandatory": "npx sol2uml class contracts -c -b BaseModule && npx sol2uml class contracts -c -b BurnModule && npx sol2uml class contracts -c -b EnforcementModule && npx sol2uml class contracts -c -b ERC20BaseModule && npx sol2uml class contracts -c -b MintModule", "uml-modules-optional": "npx sol2uml class contracts -c -b DebtBaseModule && npx sol2uml class contracts -c -b CreditEventsModule && npx sol2uml class contracts -c -b MetaTxModule && npx sol2uml class contracts -c -b SnapshotModule && npx sol2uml class contracts -c -b ValidationModule", - "uml-modules-security": "npx sol2uml class contracts -c -b AuthorizationModule && npx sol2uml class contracts -c -b OnlyDelegateCallModule", + "uml-modules-security": "npx sol2uml class contracts -c -b AuthorizationModule && npx sol2uml class contracts -c -b PauseModule", "uml-mocks": "npx sol2uml class -c contracts/mocks/RuleEngine", "size": "npx truffle run contract-size", "test:pause": "npx truffle test test/standard/modules/PauseModule.test.js test/proxy/modules/PauseModule.test.js", @@ -39,7 +39,7 @@ "test:debt": "npx truffle test test/standard/modules/DebtModule.test.js test/proxy/modules/DebtModule.test.js", "test:creditEvents": "npx truffle test test/standard/modules/CreditEventsModule.test.js test/proxy/modules/CreditEventsModule.test.js", "test:deployment": "npx truffle test test/deployment/deployment.test.js", - "test:proxy": "npx truffle test test/proxy/general/KillImplementation.test.js test/proxy/general/Proxy.test.js test/proxy/general/UpgradeProxy.test.js", + "test:proxy": "npx truffle test test/proxy/general/Proxy.test.js test/proxy/general/UpgradeProxy.test.js", "test:metaTx": "npx truffle test test/standard/modules/MetaTxModule.test.js test/proxy/modules/MetaTxModule.test.js", "test:hardhat:burn": "npx hardhat test test/standard/modules/BurnModule.test.js test/proxy/modules/BurnModule.test.js", "test:hardhat:mint": "npx hardhat test test/standard/modules/MintModule.test.js test/proxy/modules/MintModule.test.js", @@ -53,7 +53,7 @@ "test:hardhat:snapshot": "npx hardhat test test/standard/modules/SnapshotModule.test.js test/proxy/modules/SnapshotModule.test.js", "test:hardhat:enforcement": "npx hardhat test test/standard/modules/EnforcementModule.test.js test/proxy/modules/EnforcementModule.test.js", "test:hardhat:metaTx": "npx hardhat test test/standard/modules/MetaTxModule.test.js test/proxy/modules/MetaTxModule.test.js", - "test:hardhat:proxy": "npx hardhat test test/proxy/general/KillImplementation.test.js test/proxy/general/Proxy.test.js test/proxy/general/UpgradeProxy.test.js", + "test:hardhat:proxy": "npx hardhat test test/proxy/general/Proxy.test.js test/proxy/general/UpgradeProxy.test.js", "test:hardhat:deployment": "npx hardhat test test/deployment/deployment.test.js" }, "repository": { diff --git a/test/common/BaseModuleCommon.js b/test/common/BaseModuleCommon.js index a8d02117..e3d1ef75 100644 --- a/test/common/BaseModuleCommon.js +++ b/test/common/BaseModuleCommon.js @@ -124,35 +124,6 @@ function BaseModuleCommon (owner, address1, address2, address3, proxyTest) { // Assert (await this.cmtat.flag()).should.be.bignumber.equal(this.flag.toString()) }) - it('testAdminCanKillContract', async function () { - // Arrange - Assert - await web3.eth.getCode(this.cmtat.address).should.not.equal('0x') - // Act - await this.cmtat.kill({ from: owner }); - // Assert - // TODO: Check if the ethers inside the contract is sent to the right address - // A destroyed contract has a bytecode size of 0. - (await web3.eth.getCode(this.cmtat.address)).should.equal('0x') - try { - await this.cmtat.terms() - } catch (e) { - e.message.should.equal( - "Returned values aren't valid, did it run Out of Gas? You might also see this error if you are not using the correct ABI for the contract you are retrieving data from, requesting data from a block number that does not exist, or querying a node which is not fully synced." - ) - } - }) - it('testCannotNonAdminKillContract', async function () { - // Act - await expectRevertCustomError( - this.cmtat.kill({ from: address1 }), - 'AccessControlUnauthorizedAccount', - [address1, DEFAULT_ADMIN_ROLE] - ); - // Assert - (await this.cmtat.terms()).should.equal('https://cmta.ch') - // The contract is not destroyed, so the contract has a bytecode size different from zero. - await web3.eth.getCode(this.cmtat.address).should.not.equal('0x') - }) }) } module.exports = BaseModuleCommon diff --git a/test/common/PauseModuleCommon.js b/test/common/PauseModuleCommon.js index 43a56c18..ae1a0ce1 100644 --- a/test/common/PauseModuleCommon.js +++ b/test/common/PauseModuleCommon.js @@ -1,8 +1,8 @@ -const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers') +const { expectEvent } = require('@openzeppelin/test-helpers') const { expectRevertCustomError } = require('../../openzeppelin-contracts-upgradeable/test/helpers/customError.js') -const { PAUSER_ROLE } = require('../utils') +const { PAUSER_ROLE, DEFAULT_ADMIN_ROLE } = require('../utils') const { should } = require('chai').should() function PauseModuleCommon (admin, address1, address2, address3) { @@ -13,9 +13,10 @@ function PauseModuleCommon (admin, address1, address2, address3) { it('testCanBePausedByAdmin', async function () { const AMOUNT_TO_TRANSFER = 10 // Act - this.logs = await this.cmtat.pause({ from: admin }) + this.logs = await this.cmtat.pause({ from: admin }); // Assert + (await this.cmtat.paused()).should.equal(true) // emits a Paused event expectEvent(this.logs, 'Paused', { account: admin }) // Transfer is reverted @@ -32,9 +33,10 @@ function PauseModuleCommon (admin, address1, address2, address3) { await this.cmtat.grantRole(PAUSER_ROLE, address1, { from: admin }) // Act - this.logs = await this.cmtat.pause({ from: address1 }) + this.logs = await this.cmtat.pause({ from: address1 }); // Assert + (await this.cmtat.paused()).should.equal(true) // emits a Paused event expectEvent(this.logs, 'Paused', { account: address1 }) // Transfer is reverted @@ -43,6 +45,7 @@ function PauseModuleCommon (admin, address1, address2, address3) { 'CMTAT_InvalidTransfer', [address1, address2, AMOUNT_TO_TRANSFER] ) + }) it('testCannotBePausedByNonPauser', async function () { @@ -59,9 +62,10 @@ function PauseModuleCommon (admin, address1, address2, address3) { await this.cmtat.pause({ from: admin }) // Act - this.logs = await this.cmtat.unpause({ from: admin }) + this.logs = await this.cmtat.unpause({ from: admin }); // Assert + (await this.cmtat.paused()).should.equal(false) // emits a Unpaused event expectEvent(this.logs, 'Unpaused', { account: admin }) // Transfer works @@ -74,9 +78,10 @@ function PauseModuleCommon (admin, address1, address2, address3) { await this.cmtat.grantRole(PAUSER_ROLE, address1, { from: admin }) // Act - this.logs = await this.cmtat.unpause({ from: address1 }) + this.logs = await this.cmtat.unpause({ from: address1 }); // Assert + (await this.cmtat.paused()).should.equal(false) // emits a Unpaused event expectEvent(this.logs, 'Unpaused', { account: address1 }) // Transfer works @@ -147,5 +152,43 @@ function PauseModuleCommon (admin, address1, address2, address3) { ) }) }) + + context('DeactivateContract', function () { + /** + The admin is assigned the PAUSER role when the contract is deployed + */ + it('testCanDeactivatedByAdmin', async function () { + const AMOUNT_TO_TRANSFER = 10 + // Act + this.logs = await this.cmtat.deactivateContract({ from: admin }); + + // Assert + // Contract is in pause state + (await this.cmtat.paused()).should.equal(true) + // emits a Deactivated event + expectEvent(this.logs, 'Deactivated', { account: admin }) + // Transfer is reverted because contract is in paused state + await expectRevertCustomError( + this.cmtat.transfer(address2, AMOUNT_TO_TRANSFER, { from: address1 }), + 'CMTAT_InvalidTransfer', + [address1, address2, AMOUNT_TO_TRANSFER] + ) + + // Unpause is reverted + await expectRevertCustomError(this.cmtat.unpause({ from: admin }), + 'CMTAT_PauseModule_ContractIsDeactivated', + [] + ) + }) + + it('testCannotBeDeactivatedByNonAdmin', async function () { + // Act + await expectRevertCustomError( + this.cmtat.deactivateContract({ from: address1 }), + 'AccessControlUnauthorizedAccount', + [address1, DEFAULT_ADMIN_ROLE] + ) + }) + }) } module.exports = PauseModuleCommon diff --git a/test/proxy/general/KillImplementation.test.js b/test/proxy/general/KillImplementation.test.js deleted file mode 100644 index cc7bd334..00000000 --- a/test/proxy/general/KillImplementation.test.js +++ /dev/null @@ -1,45 +0,0 @@ -/** - * We use a different version of the CMTAT where we have removed the check of access control on the kill function - * The goal is to verify if the modifier onlyDelegateCall works as intended - * - */ -const { expectRevert } = require('@openzeppelin/test-helpers') -const { deployCMTATProxyWithKillTest } = require('../../deploymentUtils') -const { - expectRevertCustomError -} = require('../../../openzeppelin-contracts-upgradeable/test/helpers/customError.js') -const { should } = require('chai').should() -const { deployProxy, erc1967 } = require('@openzeppelin/truffle-upgrades') -const { ethers, upgrades } = require('hardhat') -const CMTAT_KILL_TEST = artifacts.require('CMTAT_KILL_TEST') - -contract('Proxy - Security Test', function ([_, admin, deployerAddress]) { - beforeEach(async function () { - this.flag = 5 - // Contract to deploy: CMTAT_KILL_TEST - this.CMTAT_PROXY = await deployCMTATProxyWithKillTest( - _, - admin, - deployerAddress - ) - const implementationContractAddress = - await upgrades.erc1967.getImplementationAddress( - this.CMTAT_PROXY.address, - { - from: admin - } - ) - this.implementationContract = await CMTAT_KILL_TEST.at( - implementationContractAddress - ) - }) - context('Implementation contract', function () { - it('testCannotKillTheImplementationContract', async function () { - await expectRevertCustomError( - this.implementationContract.kill({ from: admin }), - 'CMTAT_OnlyDelegateCallModule_DirectCallToImplementation', - [] - ) - }) - }) -}) diff --git a/test/proxy/general/Proxy.test.js b/test/proxy/general/Proxy.test.js index 1727f9ce..63dc5148 100644 --- a/test/proxy/general/Proxy.test.js +++ b/test/proxy/general/Proxy.test.js @@ -10,7 +10,7 @@ const { expectRevertCustomError } = require('../../../openzeppelin-contracts-upgradeable/test/helpers/customError.js') const CMTAT = artifacts.require('CMTAT_PROXY') -const { DEFAULT_ADMIN_ROLE } = require('../../utils') +const { DEFAULT_ADMIN_ROLE, PAUSER_ROLE } = require('../../utils') const { ZERO_ADDRESS } = require('../../utils') const DECIMAL = 0 const { deployCMTATProxy, DEPLOYMENT_FLAG } = require('../../deploymentUtils') @@ -52,9 +52,9 @@ contract( [] ) await expectRevertCustomError( - this.implementationContract.kill({ from: attacker }), + this.implementationContract.pause({ from: attacker }), 'AccessControlUnauthorizedAccount', - [attacker, DEFAULT_ADMIN_ROLE] + [attacker, PAUSER_ROLE] ) }) // Here the argument to indicate if it is deployed with a proxy, set at true by the attacker @@ -76,21 +76,11 @@ contract( [] ) await expectRevertCustomError( - this.implementationContract.kill({ from: attacker }), + this.implementationContract.pause({ from: attacker }), 'AccessControlUnauthorizedAccount', - [attacker, DEFAULT_ADMIN_ROLE] + [attacker, PAUSER_ROLE] ) }) }) - context('Admin', function () { - it('testCannotKillTheImplementationContractByAdmin', async function () { - await expectRevertCustomError( - this.implementationContract.kill({ from: admin }), - 'AccessControlUnauthorizedAccount', - [admin, DEFAULT_ADMIN_ROLE] - ); - (await this.implementationContract.terms()).should.equal('') - }) - }) } ) From 2eee5372330df91c2e93aee87a679ae55894749d Mon Sep 17 00:00:00 2001 From: Ryan Sauge Date: Fri, 1 Sep 2023 13:43:52 +0200 Subject: [PATCH 2/5] PauseModule: add function to get the value of isDeactivated --- contracts/modules/security/PauseModule.sol | 8 ++++++-- test/common/PauseModuleCommon.js | 5 ++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/contracts/modules/security/PauseModule.sol b/contracts/modules/security/PauseModule.sol index 0a14ab51..336c6332 100644 --- a/contracts/modules/security/PauseModule.sol +++ b/contracts/modules/security/PauseModule.sol @@ -16,7 +16,7 @@ import "./AuthorizationModule.sol"; abstract contract PauseModule is PausableUpgradeable, AuthorizationModule { string internal constant TEXT_TRANSFER_REJECTED_PAUSED = "All transfers paused"; - bool isDeactivated; + bool private isDeactivated; event Deactivated(address account); function __PauseModule_init(address admin) internal onlyInitializing { @@ -70,7 +70,7 @@ abstract contract PauseModule is PausableUpgradeable, AuthorizationModule { } /** - @notice destroys the contract and send the remaining ethers in the contract to the sender + @notice p Warning: the operation is irreversible, be careful */ /// @custom:oz-upgrades-unsafe-allow selfdestruct @@ -83,5 +83,9 @@ abstract contract PauseModule is PausableUpgradeable, AuthorizationModule { emit Deactivated(_msgSender()); } + function deactivated() view public returns (bool){ + return isDeactivated; + } + uint256[50] private __gap; } diff --git a/test/common/PauseModuleCommon.js b/test/common/PauseModuleCommon.js index ae1a0ce1..608eceb2 100644 --- a/test/common/PauseModuleCommon.js +++ b/test/common/PauseModuleCommon.js @@ -163,6 +163,7 @@ function PauseModuleCommon (admin, address1, address2, address3) { this.logs = await this.cmtat.deactivateContract({ from: admin }); // Assert + (await this.cmtat.deactivated()).should.equal(true); // Contract is in pause state (await this.cmtat.paused()).should.equal(true) // emits a Deactivated event @@ -187,7 +188,9 @@ function PauseModuleCommon (admin, address1, address2, address3) { this.cmtat.deactivateContract({ from: address1 }), 'AccessControlUnauthorizedAccount', [address1, DEFAULT_ADMIN_ROLE] - ) + ); + // Assert + (await this.cmtat.deactivated()).should.equal(false) }) }) } From 2572a22a512678c3e4a7c1df0108ff6c35a76f06 Mon Sep 17 00:00:00 2001 From: Ryan Sauge Date: Mon, 4 Sep 2023 09:15:34 +0200 Subject: [PATCH 3/5] Update test for EnforcementModule --- test/common/EnforcementModuleCommon.js | 32 +++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/test/common/EnforcementModuleCommon.js b/test/common/EnforcementModuleCommon.js index fece3f0d..98aba5fd 100644 --- a/test/common/EnforcementModuleCommon.js +++ b/test/common/EnforcementModuleCommon.js @@ -1,4 +1,4 @@ -const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers') +const { expectEvent } = require('@openzeppelin/test-helpers') const { should } = require('chai').should() const { ENFORCER_ROLE } = require('../utils') const { @@ -17,13 +17,13 @@ function EnforcementModuleCommon (owner, address1, address2, address3) { // Arrange - Assert (await this.cmtat.frozen(address1)).should.equal(false); // Act - ({ logs: this.logs } = await this.cmtat.freeze(address1, reasonFreeze, { + this.logs = await this.cmtat.freeze(address1, reasonFreeze, { from: owner - })); + }); // Assert (await this.cmtat.frozen(address1)).should.equal(true) // emits a Freeze event - expectEvent.inLogs(this.logs, 'Freeze', { + expectEvent(this.logs, 'Freeze', { enforcer: owner, owner: address1, reasonIndexed: web3.utils.keccak256(reasonFreeze), @@ -35,13 +35,13 @@ function EnforcementModuleCommon (owner, address1, address2, address3) { // Arrange - Assert (await this.cmtat.frozen(address1)).should.equal(false); // Act - ({ logs: this.logs } = await this.cmtat.freeze(address1, '', { + this.logs = await this.cmtat.freeze(address1, '', { from: owner - })); + }); // Assert (await this.cmtat.frozen(address1)).should.equal(true) // emits a Freeze event - expectEvent.inLogs(this.logs, 'Freeze', { + expectEvent(this.logs, 'Freeze', { enforcer: owner, owner: address1, // see https://ethereum.stackexchange.com/questions/35103/keccak-hash-of-null-values-result-in-different-hashes-for-different-types @@ -57,14 +57,14 @@ function EnforcementModuleCommon (owner, address1, address2, address3) { // Arrange - Assert (await this.cmtat.frozen(address1)).should.equal(false); // Act - ({ logs: this.logs } = await this.cmtat.freeze(address1, reasonFreeze, { + this.logs = await this.cmtat.freeze(address1, reasonFreeze, { from: address2 - })); + }); // Assert (await this.cmtat.frozen(address1)).should.equal(true) // emits a Freeze event - expectEvent.inLogs(this.logs, 'Freeze', { + expectEvent(this.logs, 'Freeze', { enforcer: address2, owner: address1, reasonIndexed: web3.utils.keccak256(reasonFreeze), @@ -78,16 +78,16 @@ function EnforcementModuleCommon (owner, address1, address2, address3) { // Arrange - Assert (await this.cmtat.frozen(address1)).should.equal(true); // Act - ({ logs: this.logs } = await this.cmtat.unfreeze( + this.logs = await this.cmtat.unfreeze( address1, reasonUnfreeze, { from: owner } - )); + ); // Assert (await this.cmtat.frozen(address1)).should.equal(false) - expectEvent.inLogs(this.logs, 'Unfreeze', { + expectEvent(this.logs, 'Unfreeze', { enforcer: owner, owner: address1, reasonIndexed: web3.utils.keccak256(reasonUnfreeze), @@ -102,15 +102,15 @@ function EnforcementModuleCommon (owner, address1, address2, address3) { // Arrange - Assert (await this.cmtat.frozen(address1)).should.equal(true); // Act - ({ logs: this.logs } = await this.cmtat.unfreeze( + this.logs = await this.cmtat.unfreeze( address1, reasonUnfreeze, { from: address2 } - )); + ); // Assert (await this.cmtat.frozen(address1)).should.equal(false) // emits an Unfreeze event - expectEvent.inLogs(this.logs, 'Unfreeze', { + expectEvent(this.logs, 'Unfreeze', { enforcer: address2, owner: address1, reasonIndexed: web3.utils.keccak256(reasonUnfreeze), From 50ddea20f74d8871fc1c6c9c7b168c56bc4ad22b Mon Sep 17 00:00:00 2001 From: Ryan Sauge Date: Mon, 4 Sep 2023 09:47:46 +0200 Subject: [PATCH 4/5] Improve comments for PauseModule --- contracts/modules/security/PauseModule.sol | 30 ++++++++++++++-------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/contracts/modules/security/PauseModule.sol b/contracts/modules/security/PauseModule.sol index c210651b..ee39ce16 100644 --- a/contracts/modules/security/PauseModule.sol +++ b/contracts/modules/security/PauseModule.sol @@ -7,7 +7,10 @@ import "../../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Initia import "./AuthorizationModule.sol"; /** - * @dev ERC20 token with pausable token transfers, minting and burning. + * + * @dev Put in pause or deactivate the contract + * The issuer must be able to “pause” the smart contract, + * to prevent execution of transactions on the distributed ledger until the issuer puts an end to the pause. * * Useful for scenarios such as preventing trades until the end of an evaluation * period, or having an emergency switch for freezing all token transfers in the @@ -41,22 +44,21 @@ abstract contract PauseModule is PausableUpgradeable, AuthorizationModule { } /** - * @dev Pauses all token transfers. - * - * See {ERC20Pausable} and {Pausable-_pause}. + * @notice Pauses all token transfers. + * @dev See {ERC20Pausable} and {Pausable-_pause}. * * Requirements: * * - the caller must have the `PAUSER_ROLE`. + * */ function pause() public onlyRole(PAUSER_ROLE) { _pause(); } /** - * @dev Unpauses all token transfers. - * - * See {ERC20Pausable} and {Pausable-_unpause}. + * @notice Unpauses all token transfers. + * @dev See {ERC20Pausable} and {Pausable-_unpause}. * * Requirements: * @@ -69,9 +71,14 @@ abstract contract PauseModule is PausableUpgradeable, AuthorizationModule { _unpause(); } - /** - @notice p - Warning: the operation is irreversible, be careful + /** + * @notice deactivate the contract + * Warning: the operation is irreversible, be careful + * @dev + * Emits a {Deactivated} event indicating that the contract has been deactivated. + * Requirements: + * + * - the caller must have the `DEFAULT_ADMIN_ROLE`. */ /// @custom:oz-upgrades-unsafe-allow selfdestruct function deactivateContract() @@ -83,6 +90,9 @@ abstract contract PauseModule is PausableUpgradeable, AuthorizationModule { emit Deactivated(_msgSender()); } + /** + * @notice Returns true if the contract is deactivated, and false otherwise. + */ function deactivated() view public returns (bool){ return isDeactivated; } From 25653345b5b4bb59558c9b5a9e8f0e8d83354727 Mon Sep 17 00:00:00 2001 From: Ryan Sauge Date: Mon, 4 Sep 2023 09:51:09 +0200 Subject: [PATCH 5/5] Update .gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 4d39e1d4..4ec8db2f 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,7 @@ node_modules build coverage coverage.json +bin/* #exception !doc/general/test/coverage /.openzeppelin