Improve test for proxy and AuthorizationEngine, small fix for AE, imp…

…rove doc

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
 

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

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);
 

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;
+        }
+    }
 }

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) {

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;

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`.
      */

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

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
+      ])
     })
 
     //

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])
   }

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,

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(

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 }),

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) {
       []
     )
   })
-})
+  })

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, {