chore: [C4] Add Gas optimization fixes (#235)

* chore: [C4] Add Gas optimization fixes * chore: add natspec about `msg.value` in delegatecall loop * test: add more test to pass coverage
ERC725Alliance · Sep 26, 2023 · 71db6b4 · 71db6b4
1 parent f32e4c1
commit 71db6b4
Show file tree

Hide file tree

Showing 3 changed files with 10 additions and 2 deletions.
diff --git a/implementations/contracts/ERC725XCore.sol b/implementations/contracts/ERC725XCore.sol
@@ -62,6 +62,9 @@ abstract contract ERC725XCore is OwnableUnset, ERC165, IERC725X {
      * - SHOULD only be callable by the {owner} of the contract.
      * - The contract MUST have in its balance **at least the sum of all the `values`** to transfer and execute successfully each calldata payloads.
      *
+     * @custom:warning
+     * - The `msg.value` should not be trusted for any method called with `operationType`: `DELEGATECALL` (4).
+     *
      * @custom:events
      * - {Executed} event, when a call is made with `operationType` 0 (CALL), 3 (STATICCALL) or 4 (DELEGATECALL)
      * - {ContractCreated} event, when deploying a contract with `operationType` 1 (CREATE) or 2 (CREATE2)

diff --git a/implementations/contracts/custom/OwnableUnset.sol b/implementations/contracts/custom/OwnableUnset.sol
@@ -67,9 +67,8 @@ abstract contract OwnableUnset {
      */
     function _setOwner(address newOwner) internal virtual {
         if (newOwner != owner()) {
-            address oldOwner = _owner;
+            emit OwnershipTransferred(_owner, newOwner);
             _owner = newOwner;
-            emit OwnershipTransferred(oldOwner, newOwner);
         }
     }
 }
diff --git a/implementations/test/ERC725.test.ts b/implementations/test/ERC725.test.ts
@@ -6,6 +6,8 @@ import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers';
 // types
 import { ERC725, ERC725__factory, ERC725Init__factory } from '../types';
 
+import { INTERFACE_ID } from '../constants';
+
 import { deployProxy } from './fixtures';
 
 type ERC725DeployParams = {
@@ -99,6 +101,10 @@ describe('ERC725', () => {
         context = await buildTestContext();
       });
 
+      it('should have registered the ERC725X interface', async () => {
+        expect(await context.erc725.supportsInterface(INTERFACE_ID.ERC725X));
+      });
+
       it('should revert when initializing with address(0) as owner', async () => {
         await expect(
           context.erc725['initialize(address)'](ethers.constants.AddressZero),