From 465220c9a33cdc8a1e704e101b8c6b16f2ec17c3 Mon Sep 17 00:00:00 2001 From: Ryan Sauge Date: Fri, 26 Jan 2024 13:15:38 +0100 Subject: [PATCH] Improve test for proxy and AuthorizationEngine, small fix for AE, improve doc --- README.md | 12 +------- contracts/deployment/CMTAT_TP_FACTORY.sol | 1 - .../engine/IAuthorizationEngine.sol | 9 +++++- contracts/mocks/AuthorizationEngineMock.sol | 26 +++++++++++++++-- .../internal/ERC20SnapshotModuleInternal.sol | 2 +- .../internal/base/SnapshotModuleBase.sol | 4 +-- .../modules/security/AuthorizationModule.sol | 9 ++---- ...ationModuleSetAuthorizationEngineCommon.js | 28 +++++++++++++++++++ ...RC20SnapshotModuleCommonGetNextSnapshot.js | 10 +++++++ .../ERC20SnapshotModuleUtils.js | 1 + test/common/EnforcementModuleCommon.js | 5 +++- test/common/PauseModuleCommon.js | 4 ++- .../ValidationModuleCommon.js | 25 ++++++++++++++--- test/deployment/deployment.test.js | 4 +-- test/proxy/general/Beacon.test.js | 25 +++++++++++------ test/proxy/general/Transparent.test.js | 24 +++++++++------- 16 files changed, 136 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index 08b39597..d261ceaa 100644 --- a/README.md +++ b/README.md @@ -109,12 +109,10 @@ Generally, these modules are not required to be compliant with the CMTA specific | Name | Documentation | Main File | | ----------------- | ------------------------------------------------------------ | ------------------------------------------------------------ | | MetaTxModule | [metatx.md](doc/modules/presentation/extensions/metatx.md) | [MetaTxModule.sol](./contracts/modules/wrapper/extensions/MetaTxModule.sol) | -| SnapshotModule* | [snapshot.md](doc/modules/presentation/extensions/snapshot.md) | [SnapshotModule.sol](./contracts/modules/wrapper/extensions/SnapshotModule.sol) | +| SnapshotModule | [snapshot.md](doc/modules/presentation/extensions/snapshot.md) | [SnapshotModule.sol](./contracts/modules/wrapper/extensions/SnapshotModule.sol) | | creditEventModule | [creditEvents.md](doc/modules/presentation/extensions/Debt/creditEvents.md) | [CreditEventsModule.sol](./contracts/modules/wrapper/extensions/DebtModule/CreditEventsModule.sol) | | DebtBaseModule | [debtBase.md](doc/modules/presentation/extensions/Debt/debtBase.md) | [DebtBaseModule.sol](./contracts/modules/wrapper/extensions/DebtModule/DebtBaseModule.sol) | -*not imported by default - ### Security | Name | Documentation | Main File | @@ -123,14 +121,6 @@ Generally, these modules are not required to be compliant with the CMTA specific -### SnapshotModule - -This module designed for future support of dividend and interest distribution, was not covered by the audit made by ABDK and it is no longer imported by default inside the CMTAT. - -If you want to add this module, you have to uncomment the specific lines "SnapshotModule" in [CMTAT_BASE.sol](./contracts/modules/CMTAT_BASE.sol). - -A CMTAT version inheriting from the SnapshotModule and used for **testing** purpose is available in [CMTATSnapshotStandaloneTest.sol](./contracts/test/CMTATSnapshot/CMTATSnapshotStandaloneTest.sol) and [CMTATSnapshotProxyTest.sol](./contracts/test/CMTATSnapshot/CMTATSnapshotProxyTest.sol). - ## Security diff --git a/contracts/deployment/CMTAT_TP_FACTORY.sol b/contracts/deployment/CMTAT_TP_FACTORY.sol index a79c0570..1cf37ab3 100644 --- a/contracts/deployment/CMTAT_TP_FACTORY.sol +++ b/contracts/deployment/CMTAT_TP_FACTORY.sol @@ -20,7 +20,6 @@ contract CMTAT_TP_FACTORY is AccessControl { address public immutable logic; address[] public cmtatsList; - /** * @param logic_ contract implementation * @param factoryAdmin admin diff --git a/contracts/interfaces/engine/IAuthorizationEngine.sol b/contracts/interfaces/engine/IAuthorizationEngine.sol index f49c9045..7c65c75a 100644 --- a/contracts/interfaces/engine/IAuthorizationEngine.sol +++ b/contracts/interfaces/engine/IAuthorizationEngine.sol @@ -6,7 +6,14 @@ interface IAuthorizationEngine { /** * @dev Returns true if the operation is a success, and false otherwise. */ - function operateOnAuthorization( + function operateOnGrantRole( + bytes32 role, address account + ) external returns (bool isValid); + + /** + * @dev Returns true if the operation is a success, and false otherwise. + */ + function operateOnRevokeRole( bytes32 role, address account ) external returns (bool isValid); diff --git a/contracts/mocks/AuthorizationEngineMock.sol b/contracts/mocks/AuthorizationEngineMock.sol index 1a1f18ac..e6dad25c 100644 --- a/contracts/mocks/AuthorizationEngineMock.sol +++ b/contracts/mocks/AuthorizationEngineMock.sol @@ -9,9 +9,9 @@ import "../interfaces/engine/IAuthorizationEngine.sol"; */ contract AuthorizationEngineMock is IAuthorizationEngine { address nextAdmin; - + bool revokeAdminRoleAuthorized; constructor() { - + revokeAdminRoleAuthorized = true; } /* @@ -22,11 +22,16 @@ contract AuthorizationEngineMock is IAuthorizationEngine { nextAdmin = newAdmin; } + function setRevokeAdminRoleAuthorized(bool + newValue) external { + revokeAdminRoleAuthorized = newValue; + } + /* * @dev * Warning: if you want to use this mock, you have to restrict the access to this function through an an access control */ - function operateOnAuthorization( + function operateOnGrantRole( bytes32 role, address account ) external returns (bool isValid){ if(role == 0x0 && account == nextAdmin && account != address(0x0)){ @@ -37,4 +42,19 @@ contract AuthorizationEngineMock is IAuthorizationEngine { return false; } } + + /* + * @dev + * Warning: if you want to use this mock, you have to restrict the access to this function through an an access control + */ + function operateOnRevokeRole( + bytes32 role, address /*account*/ + ) external view returns (bool isValid){ + if(role == 0x0){ + return revokeAdminRoleAuthorized; + } else{ + // the tests will fail if this branch is taken + return !revokeAdminRoleAuthorized; + } + } } diff --git a/contracts/modules/internal/ERC20SnapshotModuleInternal.sol b/contracts/modules/internal/ERC20SnapshotModuleInternal.sol index 8537987a..ad6c7b9b 100644 --- a/contracts/modules/internal/ERC20SnapshotModuleInternal.sol +++ b/contracts/modules/internal/ERC20SnapshotModuleInternal.sol @@ -77,7 +77,7 @@ abstract contract ERC20SnapshotModuleInternal is SnapshotModuleBase, ERC20Upgrad } /** - * @notice Return snapshotBalanceOf and snapshotTotalSupply to avoid multiple calls + * @notice Return snapshotBalanceOf and snapshotTotalSupply to avoid multiple calls * @return ownerBalance , totalSupply - see snapshotBalanceOf and snapshotTotalSupply */ function getSnapshotInfoBatch(uint256 time, address owner) public view returns (uint256 ownerBalance, uint256 totalSupply) { diff --git a/contracts/modules/internal/base/SnapshotModuleBase.sol b/contracts/modules/internal/base/SnapshotModuleBase.sol index 920ddb0c..59da356f 100644 --- a/contracts/modules/internal/base/SnapshotModuleBase.sol +++ b/contracts/modules/internal/base/SnapshotModuleBase.sol @@ -257,8 +257,8 @@ abstract contract SnapshotModuleBase is Initializable { } /** - * @dev - * Get all snapshots + * + * @notice Get all snapshots */ function getAllSnapshots() public view returns (uint256[] memory) { return _scheduledSnapshots; diff --git a/contracts/modules/security/AuthorizationModule.sol b/contracts/modules/security/AuthorizationModule.sol index 81738160..a270a6ac 100644 --- a/contracts/modules/security/AuthorizationModule.sol +++ b/contracts/modules/security/AuthorizationModule.sol @@ -50,7 +50,6 @@ abstract contract AuthorizationModule is AccessControlUpgradeable { } - /* * @notice set an authorizationEngine if not already set * @dev once an AuthorizationEngine is set, it is not possible to unset it @@ -67,7 +66,7 @@ abstract contract AuthorizationModule is AccessControlUpgradeable { function grantRole(bytes32 role, address account) public override onlyRole(getRoleAdmin(role)) { if (address(authorizationEngine) != address (0)) { - bool result = authorizationEngine.operateOnAuthorization(role, account); + bool result = authorizationEngine.operateOnGrantRole(role, account); if(!result) { // Operation rejected by the authorizationEngine revert Errors.CMTAT_AuthorizationModule_InvalidAuthorization(); @@ -76,12 +75,9 @@ abstract contract AuthorizationModule is AccessControlUpgradeable { return AccessControlUpgradeable.grantRole(role, account); } - - - function revokeRole(bytes32 role, address account) public override onlyRole(getRoleAdmin(role)) { if (address(authorizationEngine) != address (0)) { - bool result = authorizationEngine.operateOnAuthorization(role, account); + bool result = authorizationEngine.operateOnRevokeRole(role, account); if(!result) { // Operation rejected by the authorizationEngine revert Errors.CMTAT_AuthorizationModule_InvalidAuthorization(); @@ -90,7 +86,6 @@ abstract contract AuthorizationModule is AccessControlUpgradeable { return AccessControlUpgradeable.revokeRole(role, account); } - /** * @dev Returns `true` if `account` has been granted `role`. */ diff --git a/test/common/AuthorizationModule/AuthorizationModuleSetAuthorizationEngineCommon.js b/test/common/AuthorizationModule/AuthorizationModuleSetAuthorizationEngineCommon.js index 894d9dc0..27a9b28a 100644 --- a/test/common/AuthorizationModule/AuthorizationModuleSetAuthorizationEngineCommon.js +++ b/test/common/AuthorizationModule/AuthorizationModuleSetAuthorizationEngineCommon.js @@ -70,6 +70,34 @@ function AuthorizationModuleSetAuthorizationEngineCommon(admin, address1, author [] ) }) + + it('testCanRevokeAdminIfAuthorizedByTheEngine', async function () { + // Arrange + await this.authorizationEngineMock.authorizeAdminChange(address1) + // Act + await this.cmtat.setAuthorizationEngine(this.authorizationEngineMock.address, { from: admin}) + // Assert + this.logs = await this.cmtat.revokeRole(DEFAULT_ADMIN_ROLE, address1, { + from: admin + }); + // Assert + (await this.cmtat.hasRole(DEFAULT_ADMIN_ROLE, address1)).should.equal(false) + }) + + // Mock + it('testCannotRevokeAdminIfNotAuthorizedByTheEngine', async function () { + // Arrange + await this.cmtat.setAuthorizationEngine(this.authorizationEngineMock.address, { from: admin }) + await this.authorizationEngineMock.setRevokeAdminRoleAuthorized(false); + // Act + await expectRevertCustomError( + this.cmtat.revokeRole(DEFAULT_ADMIN_ROLE, address1, { + from: admin + }), + 'CMTAT_AuthorizationModule_InvalidAuthorization', + [] + ) + }) }) } module.exports = AuthorizationModuleSetAuthorizationEngineCommon diff --git a/test/common/ERC20SnapshotModuleCommon/ERC20SnapshotModuleCommonGetNextSnapshot.js b/test/common/ERC20SnapshotModuleCommon/ERC20SnapshotModuleCommonGetNextSnapshot.js index 8b2ff251..a8072c2a 100644 --- a/test/common/ERC20SnapshotModuleCommon/ERC20SnapshotModuleCommonGetNextSnapshot.js +++ b/test/common/ERC20SnapshotModuleCommon/ERC20SnapshotModuleCommonGetNextSnapshot.js @@ -47,6 +47,16 @@ function ERC20SnapshotModuleCommonGetNextSnapshot ( this.snapshotTime4, this.snapshotTime5 ]) + // Act + const AllSnapshots = await this.cmtat.getAllSnapshots() + // Assert + checkArraySnapshot(AllSnapshots, [ + this.snapshotTime1, + this.snapshotTime2, + this.snapshotTime3, + this.snapshotTime4, + this.snapshotTime5 + ]) }) // diff --git a/test/common/ERC20SnapshotModuleCommon/ERC20SnapshotModuleUtils/ERC20SnapshotModuleUtils.js b/test/common/ERC20SnapshotModuleCommon/ERC20SnapshotModuleUtils/ERC20SnapshotModuleUtils.js index 094c035a..fcce2ef0 100644 --- a/test/common/ERC20SnapshotModuleCommon/ERC20SnapshotModuleUtils/ERC20SnapshotModuleUtils.js +++ b/test/common/ERC20SnapshotModuleCommon/ERC20SnapshotModuleUtils/ERC20SnapshotModuleUtils.js @@ -24,6 +24,7 @@ async function checkSnapshot (time, totalSupply, addresses, balances) { } async function checkArraySnapshot (snapshots, snapshotsValue) { + for (let i = 0; i < snapshots.length; ++i) { snapshots[i].should.be.bignumber.equal(snapshotsValue[i]) } diff --git a/test/common/EnforcementModuleCommon.js b/test/common/EnforcementModuleCommon.js index 2893e0bf..9ff50c6e 100644 --- a/test/common/EnforcementModuleCommon.js +++ b/test/common/EnforcementModuleCommon.js @@ -60,6 +60,9 @@ function EnforcementModuleCommon (owner, address1, address2, address3) { this.logs = await this.cmtat.freeze(address1, reasonFreeze, { from: address2 }); + // Act + Assert + // Act + Assert + (await this.cmtat.validateTransfer(address1, address2, 10)).should.be.equal(false); // Assert (await this.cmtat.frozen(address1)).should.equal(true) @@ -102,7 +105,7 @@ function EnforcementModuleCommon (owner, address1, address2, address3) { from: address2 }); // Assert - (await this.cmtat.frozen(address1)).should.equal(false) + (await this.cmtat.frozen(address1)).should.equal(false); // emits an Unfreeze event expectEvent(this.logs, 'Unfreeze', { enforcer: address2, diff --git a/test/common/PauseModuleCommon.js b/test/common/PauseModuleCommon.js index ed0244bc..d14d15ae 100644 --- a/test/common/PauseModuleCommon.js +++ b/test/common/PauseModuleCommon.js @@ -80,7 +80,7 @@ function PauseModuleCommon (admin, address1, address2, address3) { this.logs = await this.cmtat.unpause({ from: address1 }); // Assert - (await this.cmtat.paused()).should.equal(false) + (await this.cmtat.paused()).should.equal(false); // emits a Unpaused event expectEvent(this.logs, 'Unpaused', { account: address1 }) // Transfer works @@ -103,6 +103,8 @@ function PauseModuleCommon (admin, address1, address2, address3) { const AMOUNT_TO_TRANSFER = 10 // Act await this.cmtat.pause({ from: admin }); + // Act + Assert + (await this.cmtat.validateTransfer(address1, address2, AMOUNT_TO_TRANSFER) ).should.be.equal(false); // Assert ( await this.cmtat.detectTransferRestriction( diff --git a/test/common/ValidationModule/ValidationModuleCommon.js b/test/common/ValidationModule/ValidationModuleCommon.js index e0970a61..0def244b 100644 --- a/test/common/ValidationModule/ValidationModuleCommon.js +++ b/test/common/ValidationModule/ValidationModuleCommon.js @@ -25,11 +25,21 @@ function ValidationModuleCommon ( } }) + it('testCanValidateTransferWithoutRuleEngine', async function () { + // Arrange + await this.cmtat.setRuleEngine(ZERO_ADDRESS, { + from: admin + }); + // Act + Assert + (await this.cmtat.validateTransfer(address1, address2, 10)).should.be.equal(true) + }) + it('testCanDetectTransferRestrictionValidTransfer', async function () { // Act + Assert ( await this.cmtat.detectTransferRestriction(address1, address2, 11) - ).should.be.bignumber.equal('0') + ).should.be.bignumber.equal('0'); + (await this.cmtat.validateTransfer(address1, address2, 11)).should.be.equal(true) }) it('testCanReturnMessageValidTransfer', async function () { @@ -47,7 +57,10 @@ function ValidationModuleCommon ( address2, RULE_MOCK_AMOUNT_MAX + 1 ) - ).should.be.bignumber.equal('10') + ).should.be.bignumber.equal('10'); + + (await this.cmtat.validateTransfer(address1, address2, RULE_MOCK_AMOUNT_MAX + 1)) + .should.be.equal(false) }) it('testCanReturnMessageWithAmountTooHigh', async function () { @@ -66,7 +79,9 @@ function ValidationModuleCommon ( // ADDRESS1 may transfer tokens to ADDRESS2 it('testCanTransferAllowedByRule', async function () { - const AMOUNT_TO_TRANSFER = 11 + const AMOUNT_TO_TRANSFER = 11; + // Act + (await this.cmtat.validateTransfer(address1, address2, AMOUNT_TO_TRANSFER) ).should.be.equal(true) // Act await this.cmtat.transfer(address2, AMOUNT_TO_TRANSFER, { from: address1 @@ -85,7 +100,9 @@ function ValidationModuleCommon ( // reverts if ADDRESS1 transfers more tokens than rule allows it('testCannotTransferIfNotAllowedByRule', async function () { - const AMOUNT_TO_TRANSFER = RULE_MOCK_AMOUNT_MAX + 1 + const AMOUNT_TO_TRANSFER = RULE_MOCK_AMOUNT_MAX + 1; + // Act + (await this.cmtat.validateTransfer(address1, address2, AMOUNT_TO_TRANSFER) ).should.be.equal(false) // Act await expectRevertCustomError( this.cmtat.transfer(address2, AMOUNT_TO_TRANSFER, { from: address1 }), diff --git a/test/deployment/deployment.test.js b/test/deployment/deployment.test.js index b63de452..a1a501f5 100644 --- a/test/deployment/deployment.test.js +++ b/test/deployment/deployment.test.js @@ -7,7 +7,7 @@ const { deployCMTATProxyWithParameter, deployCMTATStandaloneWithParameter } = require('../deploymentUtils') -contract('CMTAT - Deployment', function ([_], deployer) { +contract('CMTAT - Deployment', function ([_], deployer, address1, address2) { it('testCannotDeployProxyWithAdminSetToAddressZero', async function () { this.flag = 5 const DECIMAL = 0 @@ -54,4 +54,4 @@ contract('CMTAT - Deployment', function ([_], deployer) { [] ) }) -}) + }) diff --git a/test/proxy/general/Beacon.test.js b/test/proxy/general/Beacon.test.js index 2b54105a..d3a3a200 100644 --- a/test/proxy/general/Beacon.test.js +++ b/test/proxy/general/Beacon.test.js @@ -15,13 +15,20 @@ contract( 'Proxy - Security Test', function ([_, admin, attacker, deployerAddress]) { beforeEach(async function () { - this.CMTAT_PROXY = await deployCMTATProxyImplementation(_, deployerAddress) - this.FACTORY = await CMTAT_BEACON_FACTORY.new(this.CMTAT_PROXY.address, admin, admin) + this.CMTAT_PROXY_IMPL = await deployCMTATProxyImplementation(_, deployerAddress) + this.FACTORY = await CMTAT_BEACON_FACTORY.new(this.CMTAT_PROXY_IMPL.address, admin, admin) }) - context('Attacker', function () { - // Here the argument to indicate if it is deployed with a proxy, set at false by the attacker - it('testCannotBeDeployedByAttacker', async function () { + context('FactoryDeployment', function () { + it('testCanReturnTheRightImplementation', async function () { + // Act + Assert + (await this.FACTORY.implementation()).should.equal(this.CMTAT_PROXY_IMPL.address) + }) + + }) + + context('Deploy CMTAT with Factory', function () { + it('testCannotBeDeployedByAttacker', async function () { // Act await expectRevertCustomError( this.FACTORY.deployCMTAT( @@ -39,9 +46,6 @@ contract( [attacker, CMTAT_DEPLOYER_ROLE] ) }) - }) - - context('Deploy CMTAT with Factory', function () { // Here the argument to indicate if it is deployed with a proxy, set at false by the attacker it('testCanDeployCMTATWithFactory', async function () { // Act @@ -58,10 +62,13 @@ contract( DEPLOYMENT_FLAG, { from: admin }); + // Check Id increment (this.logs.logs[1].args[1]).should.be.bignumber.equal(BN(0)) // Assert - const CMTAT_ADDRESS = this.logs.logs[1].args[0] + const CMTAT_ADDRESS = this.logs.logs[1].args[0]; + //Check address with ID + (await this.FACTORY.getAddress(0)).should.equal(CMTAT_ADDRESS) const CMTAT_TRUFFLE = await CMTAT.at(CMTAT_ADDRESS) // Act + Assert await CMTAT_TRUFFLE.mint(admin, 100, { diff --git a/test/proxy/general/Transparent.test.js b/test/proxy/general/Transparent.test.js index 7cd6b4b9..defd1c7c 100644 --- a/test/proxy/general/Transparent.test.js +++ b/test/proxy/general/Transparent.test.js @@ -16,12 +16,18 @@ contract( 'Proxy - Security Test', function ([_, admin, attacker, deployerAddress]) { beforeEach(async function () { - this.CMTAT_PROXY = await deployCMTATProxyImplementation(_, deployerAddress) - this.FACTORY = await CMTAT_TP_FACTORY.new(this.CMTAT_PROXY.address, admin) + this.CMTAT_PROXY_IMPL = await deployCMTATProxyImplementation(_, deployerAddress) + this.FACTORY = await CMTAT_TP_FACTORY.new(this.CMTAT_PROXY_IMPL.address, admin) }) - context('Attacker', function () { - // Here the argument to indicate if it is deployed with a proxy, set at false by the attacker + context('FactoryDeployment', function () { + it('testCanReturnTheRightImplementation', async function () { + // Act + Assert + (await this.FACTORY.logic()).should.equal(this.CMTAT_PROXY_IMPL.address) + }) + }) + + context('Deploy CMTAT with Factory', function () { it('testCannotBeDeployedByAttacker', async function () { // Act await expectRevertCustomError( @@ -39,11 +45,7 @@ contract( DEPLOYMENT_FLAG, { from: attacker }), 'AccessControlUnauthorizedAccount', [attacker, CMTAT_DEPLOYER_ROLE] - ) - }) - }) - - context('Deploy CMTAT with Factory', function () { + )}) // Here the argument to indicate if it is deployed with a proxy, set at false by the attacker it('testCanDeployCMTATWithFactory', async function () { // Act @@ -64,7 +66,9 @@ contract( // Assert // Check Id (this.logs.logs[1].args[1]).should.be.bignumber.equal(BN(0)) - const CMTAT_ADDRESS = this.logs.logs[1].args[0] + const CMTAT_ADDRESS = this.logs.logs[1].args[0]; + //Check address with ID + (await this.FACTORY.getAddress(0)).should.equal(CMTAT_ADDRESS) const CMTAT_TRUFFLE = await CMTAT.at(CMTAT_ADDRESS) await CMTAT_TRUFFLE.mint(admin, 100, { from: admin