From a3e112d436a1f9cc08070a8eac93449ced1deef5 Mon Sep 17 00:00:00 2001 From: Gerardo Nardelli Date: Wed, 19 Jun 2019 14:29:18 -0300 Subject: [PATCH 1/6] Add coverage to gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index ef58a67d8..a98e6dc5f 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ build flats .node* .idea +coverage From a13dc71e7661dcc17ba1d357d6128dc28b6cb365 Mon Sep 17 00:00:00 2001 From: Gerardo Nardelli Date: Wed, 19 Jun 2019 14:29:41 -0300 Subject: [PATCH 2/6] Remove not used log error file --- .node-xmlhttprequest-sync-27493 | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 .node-xmlhttprequest-sync-27493 diff --git a/.node-xmlhttprequest-sync-27493 b/.node-xmlhttprequest-sync-27493 deleted file mode 100644 index e69de29bb..000000000 From e79c237c8d1d82506183d1b9e5b7b81f2277a7be Mon Sep 17 00:00:00 2001 From: Gerardo Nardelli Date: Wed, 19 Jun 2019 15:03:14 -0300 Subject: [PATCH 3/6] Remove proxyOwner method from proxy --- contracts/IOwnedUpgradeabilityProxy.sol | 2 +- .../upgradeability/OwnedUpgradeabilityProxy.sol | 12 ++---------- .../upgradeable_contracts/OwnedUpgradeability.sol | 2 +- test/erc_to_erc/home_bridge.test.js | 6 +++--- test/erc_to_native/home_bridge.test.js | 12 ++++++------ 5 files changed, 13 insertions(+), 21 deletions(-) diff --git a/contracts/IOwnedUpgradeabilityProxy.sol b/contracts/IOwnedUpgradeabilityProxy.sol index 7da3d5c65..53385a170 100644 --- a/contracts/IOwnedUpgradeabilityProxy.sol +++ b/contracts/IOwnedUpgradeabilityProxy.sol @@ -2,5 +2,5 @@ pragma solidity 0.4.24; interface IOwnedUpgradeabilityProxy { - function proxyOwner() public view returns (address); + function upgradeabilityOwner() public view returns (address); } diff --git a/contracts/upgradeability/OwnedUpgradeabilityProxy.sol b/contracts/upgradeability/OwnedUpgradeabilityProxy.sol index 8fc51a50f..bc09be589 100644 --- a/contracts/upgradeability/OwnedUpgradeabilityProxy.sol +++ b/contracts/upgradeability/OwnedUpgradeabilityProxy.sol @@ -27,25 +27,17 @@ contract OwnedUpgradeabilityProxy is UpgradeabilityOwnerStorage, UpgradeabilityP * @dev Throws if called by any account other than the owner. */ modifier onlyProxyOwner() { - require(msg.sender == proxyOwner()); + require(msg.sender == upgradeabilityOwner()); _; } - /** - * @dev Tells the address of the proxy owner - * @return the address of the proxy owner - */ - function proxyOwner() public view returns (address) { - return upgradeabilityOwner(); - } - /** * @dev Allows the current owner to transfer control of the contract to a newOwner. * @param newOwner The address to transfer ownership to. */ function transferProxyOwnership(address newOwner) public onlyProxyOwner { require(newOwner != address(0)); - emit ProxyOwnershipTransferred(proxyOwner(), newOwner); + emit ProxyOwnershipTransferred(upgradeabilityOwner(), newOwner); setUpgradeabilityOwner(newOwner); } diff --git a/contracts/upgradeable_contracts/OwnedUpgradeability.sol b/contracts/upgradeable_contracts/OwnedUpgradeability.sol index c31b15ed2..dee478b46 100644 --- a/contracts/upgradeable_contracts/OwnedUpgradeability.sol +++ b/contracts/upgradeable_contracts/OwnedUpgradeability.sol @@ -6,7 +6,7 @@ import "../IOwnedUpgradeabilityProxy.sol"; contract OwnedUpgradeability { function upgradeabilityAdmin() public view returns (address) { - return IOwnedUpgradeabilityProxy(this).proxyOwner(); + return IOwnedUpgradeabilityProxy(this).upgradeabilityOwner(); } // Avoid using onlyProxyOwner name to prevent issues with implementation from proxy contract diff --git a/test/erc_to_erc/home_bridge.test.js b/test/erc_to_erc/home_bridge.test.js index ff610fb58..3017e4b00 100644 --- a/test/erc_to_erc/home_bridge.test.js +++ b/test/erc_to_erc/home_bridge.test.js @@ -994,16 +994,16 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { }) }) describe('#OwnedUpgradeability', async () => { - it('upgradeabilityAdmin should return the proxy owner', async () => { + it('upgradeabilityAdmin should return the upgradeabilityOwner', async () => { const homeBridgeImpl = await HomeBridge.new() const storageProxy = await EternalStorageProxy.new().should.be.fulfilled await storageProxy.upgradeTo('1', homeBridgeImpl.address).should.be.fulfilled const homeBridge = await HomeBridge.at(storageProxy.address) - const proxyOwner = await storageProxy.proxyOwner() + const upgradeabilityOwner = await storageProxy.upgradeabilityOwner() const upgradeabilityAdmin = await homeBridge.upgradeabilityAdmin() - upgradeabilityAdmin.should.be.equal(proxyOwner) + upgradeabilityAdmin.should.be.equal(upgradeabilityOwner) }) }) diff --git a/test/erc_to_native/home_bridge.test.js b/test/erc_to_native/home_bridge.test.js index 108ab3a3e..519f23a98 100644 --- a/test/erc_to_native/home_bridge.test.js +++ b/test/erc_to_native/home_bridge.test.js @@ -180,7 +180,7 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { it('can be upgraded keeping the state', async () => { const homeOwner = accounts[8] const storageProxy = await EternalStorageProxy.new().should.be.fulfilled - const proxyOwner = await storageProxy.proxyOwner() + const upgradeabilityOwner = await storageProxy.upgradeabilityOwner() const data = homeContract.contract.methods .initialize( validatorContract.address, @@ -205,7 +205,7 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { expect(await finalContract.maxPerTx()).to.be.bignumber.equal('2') expect(await finalContract.minPerTx()).to.be.bignumber.equal('1') expect(await finalContract.blockRewardContract()).to.be.equal(blockRewardContract.address) - expect(await finalContract.upgradeabilityAdmin()).to.be.equal(proxyOwner) + expect(await finalContract.upgradeabilityAdmin()).to.be.equal(upgradeabilityOwner) const homeContractV2 = await HomeBridge.new() await storageProxy.upgradeTo('2', homeContractV2.address).should.be.fulfilled @@ -217,7 +217,7 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { expect(await finalContractV2.maxPerTx()).to.be.bignumber.equal('2') expect(await finalContractV2.minPerTx()).to.be.bignumber.equal('1') expect(await finalContractV2.blockRewardContract()).to.be.equal(blockRewardContract.address) - expect(await finalContractV2.upgradeabilityAdmin()).to.be.equal(proxyOwner) + expect(await finalContractV2.upgradeabilityAdmin()).to.be.equal(upgradeabilityOwner) }) it('cant initialize with invalid arguments', async () => { false.should.be.equal(await homeContract.isInitialized()) @@ -1381,16 +1381,16 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { }) }) describe('#OwnedUpgradeability', async () => { - it('upgradeabilityAdmin should return the proxy owner', async () => { + it('upgradeabilityAdmin should return the upgradeabilityOwner', async () => { const homeBridgeImpl = await HomeBridge.new() const storageProxy = await EternalStorageProxy.new().should.be.fulfilled await storageProxy.upgradeTo('1', homeBridgeImpl.address).should.be.fulfilled const homeBridge = await HomeBridge.at(storageProxy.address) - const proxyOwner = await storageProxy.proxyOwner() + const upgradeabilityOwner = await storageProxy.upgradeabilityOwner() const upgradeabilityAdmin = await homeBridge.upgradeabilityAdmin() - upgradeabilityAdmin.should.be.equal(proxyOwner) + upgradeabilityAdmin.should.be.equal(upgradeabilityOwner) }) }) From f3c94b7841be3d0a4a694b30932cf4abe305f6d1 Mon Sep 17 00:00:00 2001 From: Gerardo Nardelli Date: Fri, 21 Jun 2019 10:11:48 -0300 Subject: [PATCH 4/6] Rename onlyProxyOwner --- contracts/upgradeability/OwnedUpgradeabilityProxy.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/upgradeability/OwnedUpgradeabilityProxy.sol b/contracts/upgradeability/OwnedUpgradeabilityProxy.sol index bc09be589..e6d24af22 100644 --- a/contracts/upgradeability/OwnedUpgradeabilityProxy.sol +++ b/contracts/upgradeability/OwnedUpgradeabilityProxy.sol @@ -26,7 +26,7 @@ contract OwnedUpgradeabilityProxy is UpgradeabilityOwnerStorage, UpgradeabilityP /** * @dev Throws if called by any account other than the owner. */ - modifier onlyProxyOwner() { + modifier onlyUpgradeabilityOwner() { require(msg.sender == upgradeabilityOwner()); _; } @@ -35,7 +35,7 @@ contract OwnedUpgradeabilityProxy is UpgradeabilityOwnerStorage, UpgradeabilityP * @dev Allows the current owner to transfer control of the contract to a newOwner. * @param newOwner The address to transfer ownership to. */ - function transferProxyOwnership(address newOwner) public onlyProxyOwner { + function transferProxyOwnership(address newOwner) public onlyUpgradeabilityOwner { require(newOwner != address(0)); emit ProxyOwnershipTransferred(upgradeabilityOwner(), newOwner); setUpgradeabilityOwner(newOwner); @@ -46,7 +46,7 @@ contract OwnedUpgradeabilityProxy is UpgradeabilityOwnerStorage, UpgradeabilityP * @param version representing the version name of the new implementation to be set. * @param implementation representing the address of the new implementation to be set. */ - function upgradeTo(uint256 version, address implementation) public onlyProxyOwner { + function upgradeTo(uint256 version, address implementation) public onlyUpgradeabilityOwner { _upgradeTo(version, implementation); } @@ -58,7 +58,7 @@ contract OwnedUpgradeabilityProxy is UpgradeabilityOwnerStorage, UpgradeabilityP * @param data represents the msg.data to bet sent in the low level call. This parameter may include the function * signature of the implementation to be called with the needed payload */ - function upgradeToAndCall(uint256 version, address implementation, bytes data) payable public onlyProxyOwner { + function upgradeToAndCall(uint256 version, address implementation, bytes data) payable public onlyUpgradeabilityOwner { upgradeTo(version, implementation); require(address(this).call.value(msg.value)(data)); } From 41f9ba8cf67e04817a7d59f7a8a865c83f30d1a8 Mon Sep 17 00:00:00 2001 From: Gerardo Nardelli Date: Fri, 21 Jun 2019 11:03:43 -0300 Subject: [PATCH 5/6] Remove upgradeabilityAdmin --- .../OwnedUpgradeability.sol | 7 +------ test/erc_to_erc/home_bridge.test.js | 13 ------------- test/erc_to_native/home_bridge.test.js | 16 ---------------- 3 files changed, 1 insertion(+), 35 deletions(-) diff --git a/contracts/upgradeable_contracts/OwnedUpgradeability.sol b/contracts/upgradeable_contracts/OwnedUpgradeability.sol index dee478b46..165d4a179 100644 --- a/contracts/upgradeable_contracts/OwnedUpgradeability.sol +++ b/contracts/upgradeable_contracts/OwnedUpgradeability.sol @@ -4,14 +4,9 @@ import "../IOwnedUpgradeabilityProxy.sol"; contract OwnedUpgradeability { - - function upgradeabilityAdmin() public view returns (address) { - return IOwnedUpgradeabilityProxy(this).upgradeabilityOwner(); - } - // Avoid using onlyProxyOwner name to prevent issues with implementation from proxy contract modifier onlyIfOwnerOfProxy() { - require(msg.sender == upgradeabilityAdmin()); + require(msg.sender == IOwnedUpgradeabilityProxy(this).upgradeabilityOwner()); _; } } diff --git a/test/erc_to_erc/home_bridge.test.js b/test/erc_to_erc/home_bridge.test.js index 3017e4b00..431de9943 100644 --- a/test/erc_to_erc/home_bridge.test.js +++ b/test/erc_to_erc/home_bridge.test.js @@ -993,19 +993,6 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => { await homeBridge.fixAssetsAboveLimits(transactionHash, true, { from: owner }).should.be.fulfilled }) }) - describe('#OwnedUpgradeability', async () => { - it('upgradeabilityAdmin should return the upgradeabilityOwner', async () => { - const homeBridgeImpl = await HomeBridge.new() - const storageProxy = await EternalStorageProxy.new().should.be.fulfilled - await storageProxy.upgradeTo('1', homeBridgeImpl.address).should.be.fulfilled - const homeBridge = await HomeBridge.at(storageProxy.address) - - const upgradeabilityOwner = await storageProxy.upgradeabilityOwner() - const upgradeabilityAdmin = await homeBridge.upgradeabilityAdmin() - - upgradeabilityAdmin.should.be.equal(upgradeabilityOwner) - }) - }) describe('#rewardableInitialize', async () => { let homeFee diff --git a/test/erc_to_native/home_bridge.test.js b/test/erc_to_native/home_bridge.test.js index 519f23a98..7317a19f1 100644 --- a/test/erc_to_native/home_bridge.test.js +++ b/test/erc_to_native/home_bridge.test.js @@ -180,7 +180,6 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { it('can be upgraded keeping the state', async () => { const homeOwner = accounts[8] const storageProxy = await EternalStorageProxy.new().should.be.fulfilled - const upgradeabilityOwner = await storageProxy.upgradeabilityOwner() const data = homeContract.contract.methods .initialize( validatorContract.address, @@ -205,7 +204,6 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { expect(await finalContract.maxPerTx()).to.be.bignumber.equal('2') expect(await finalContract.minPerTx()).to.be.bignumber.equal('1') expect(await finalContract.blockRewardContract()).to.be.equal(blockRewardContract.address) - expect(await finalContract.upgradeabilityAdmin()).to.be.equal(upgradeabilityOwner) const homeContractV2 = await HomeBridge.new() await storageProxy.upgradeTo('2', homeContractV2.address).should.be.fulfilled @@ -217,7 +215,6 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { expect(await finalContractV2.maxPerTx()).to.be.bignumber.equal('2') expect(await finalContractV2.minPerTx()).to.be.bignumber.equal('1') expect(await finalContractV2.blockRewardContract()).to.be.equal(blockRewardContract.address) - expect(await finalContractV2.upgradeabilityAdmin()).to.be.equal(upgradeabilityOwner) }) it('cant initialize with invalid arguments', async () => { false.should.be.equal(await homeContract.isInitialized()) @@ -1380,19 +1377,6 @@ contract('HomeBridge_ERC20_to_Native', async accounts => { await homeBridge.fixAssetsAboveLimits(transactionHash, true, { from: owner }).should.be.fulfilled }) }) - describe('#OwnedUpgradeability', async () => { - it('upgradeabilityAdmin should return the upgradeabilityOwner', async () => { - const homeBridgeImpl = await HomeBridge.new() - const storageProxy = await EternalStorageProxy.new().should.be.fulfilled - await storageProxy.upgradeTo('1', homeBridgeImpl.address).should.be.fulfilled - const homeBridge = await HomeBridge.at(storageProxy.address) - - const upgradeabilityOwner = await storageProxy.upgradeabilityOwner() - const upgradeabilityAdmin = await homeBridge.upgradeabilityAdmin() - - upgradeabilityAdmin.should.be.equal(upgradeabilityOwner) - }) - }) describe('#feeManager', async () => { let homeBridge From e73d2aa4e01a6f1c7b3d6f57f798dff0643b7e38 Mon Sep 17 00:00:00 2001 From: Gerardo Nardelli Date: Thu, 27 Jun 2019 14:38:40 -0300 Subject: [PATCH 6/6] Update _upgradeabilityOwner visibility --- contracts/upgradeability/UpgradeabilityOwnerStorage.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/upgradeability/UpgradeabilityOwnerStorage.sol b/contracts/upgradeability/UpgradeabilityOwnerStorage.sol index e502e3249..a6a27cf02 100644 --- a/contracts/upgradeability/UpgradeabilityOwnerStorage.sol +++ b/contracts/upgradeability/UpgradeabilityOwnerStorage.sol @@ -7,7 +7,7 @@ pragma solidity 0.4.24; */ contract UpgradeabilityOwnerStorage { // Owner of the contract - address private _upgradeabilityOwner; + address internal _upgradeabilityOwner; /** * @dev Tells the address of the owner