Update security roles on v1 contracts #116

patitonar · 2018-12-04T18:53:41Z

Relates to #111

Applied the new security roles, updated deploy script and tests.

Added upgradeSecurityRoles method to be called using upgradeToAndCall for already deployed bridge contracts that has not an owner set on initialization.

akolotov · 2018-12-04T19:48:49Z

May you prepare new gists based on the contracts you prepared in #109?

contracts/upgradeable_contracts/Validatable.sol

patitonar · 2018-12-05T14:10:31Z

I will prepare gist for the contracts. Should I also test upgrading them similar as we did on #109 ?

akolotov · 2018-12-05T14:21:36Z

I will prepare gist for the contracts. Should I also test upgrading them similar as we did on #109 ?

yes. And it should be extended with tests that we can send back tokens that exceed the limit.

patitonar · 2018-12-05T18:12:30Z

Here are the gists:

Next step is to run the suggested tests

patitonar · 2018-12-05T18:47:33Z

yes. And it should be extended with tests that we can send back tokens that exceed the limit.

Actually on v1.x contracts I don't think we have the ability to send back tokens that exceed the limit. From changes on #109, transactions that exceeded the limit are reverted. However, such kind of test can be applied to ERC-TO-NATIVE mode on changes from #110. Is that what you had on mind?

akolotov · 2018-12-05T18:58:18Z

Is that what you had on mind?

Oh! Sorry. It should be for another issue, for #110. So, for #116 you need to test the following:

deploy the contract with three different accounts as the admins
make some two ways transactions
upgrade the contract, set new owner for the bridge contract
make some two ways transactions
reset blocks and restart the bridge to check that the storage was not affected and no double spend happens
check that the bridge admin works as expected and it cannot perform operations with validators contract, cannot upgrade both the bridge and validator contracts, cannot claim tokens.
check that the validator admin works as expected and it cannot perform operations with the bridge contract, cannot upgrade both the bridge and validator contracts
check that the upgradability admin is able to perform upgrade, can claim tokens.

patitonar · 2018-12-07T17:00:40Z

@akolotov I run the tests and found one issue.
The issue is related to methods on Bridge contract using modifier onlyProxyOwner.
When I tried to run upgradeToAndCall with correct account to upgrade contract and calling upgradeSecurityRoles it failed.
I removed the onlyProxyOwner modifier and it worked correctly.

function upgradeSecurityRoles(address _newOwner) public onlyProxyOwner {
    require(owner() == address(0));
    require(_newOwner != address(0));
    setOwner(_newOwner);
}

Then when testing the upgradability admin role, I wasn't able to claim tokens.
This method uses onlyProxyOwner too.

function claimTokens(address _token, address _to) external onlyProxyOwner {
    ...
}

I run some tests to understand what was causing the issue with onlyProxyOwner. Which is defined on OwnedUpgradeabilityProxy as:

modifier onlyProxyOwner() {
    require(msg.sender == proxyOwner());
    _;
}

I found that methods defined on Bridge contracts that calls proxyOwner() the returned value is 0x0000000000000000000000000000000000000000.
But if you call the method EternalStorageProxy.methods.proxyOwner() the returned value is the correct.
I don't know exactly what is the cause of this but it seems some kind of issue of the bridge contract on accessing the state on the proxy contract.

Any ideas or suggestion on how to fix this or any other approach on how can we avoid this situation?

Besides this issue, the new roles worked as expected.

patitonar · 2018-12-07T18:09:25Z

@akolotov Found the issue, it was related to the way in how Validatable was defined.
https://github.com/poanetwork/poa-bridge-contracts/blob/f6ba43cd2d3c230826e76d67fafa7a8cfa27c6ef/contracts/upgradeable_contracts/Validatable.sol#L8

Fixing the order of inheritance made the trick

contract Validatable is OwnedUpgradeabilityProxy, EternalStorage, Ownable {

}

I pushed the fix, and updated the gists files. Also, I was able to re-test the steps that had issues, and now it worked as expected on the steps that used to fail.

akolotov · 2018-12-07T18:12:18Z

Great! Do you you know exactly the reason why the order matters?

patitonar · 2018-12-07T18:42:52Z

Unfortunately, I don't know the exact reason. I just tried having the same order as EternalStorageProxy does.
https://github.com/poanetwork/poa-bridge-contracts/blob/1f61c695df23b2a8f79d96e138c6bb5d8cd5b592/contracts/upgradeability/EternalStorageProxy.sol#L13

akolotov · 2018-12-07T20:21:13Z

Honestly, I do not see any reasons why Validatable should inherit something besides EternalStorage.

patitonar · 2018-12-10T11:44:05Z

We still need OwnedUpgradeabilityProxy for methods executed by onlyProxyOwner and Ownable for setting an owner and for methods that require onlyOwner. Do you have any suggestion on how to refactor this?

patitonar · 2018-12-10T16:23:35Z

onlyProxyOwner is also used by claimTokens method which is on ForeignBridge. So yes, I think moving OwnedUpgradeabilityProxy inheritance to HomeBridge and ForeignBridge will work. Regarding Ownable, onlyOwner is used on BasicBridge, HomeBridge and ForeignBridge. So I think moving it to BasicBridge is the best option. Is this ok?

patitonar · 2018-12-10T18:03:07Z

Updated the contracts with proposed changes. Also gists files were updated.

akolotov · 2018-12-11T12:58:15Z

Updated the contracts with proposed changes. Also gists files were updated.

Did you perform tests as described above after the changes?

patitonar · 2018-12-11T13:26:59Z

Did you perform tests as described above after the changes?

Not yet, I'll perform the test and share the results

# Conflicts: # contracts/upgradeable_contracts/U_ForeignBridge.sol # contracts/upgradeable_contracts/U_HomeBridge.sol # deploy/src/foreign.js # deploy/src/home.js # migrations/3_upgradeable_deployment.js # test/foreign_bridge_test.js # test/home_bridge_test.js

patitonar · 2018-12-17T19:47:27Z

I updated the changes with solution similar to v2 contracts here 76f7793
Also updated gist files.
Home: https://gist.github.com/patitonar/8caa3631e5a89b85ff3269a27c429813
Foreign: https://gist.github.com/patitonar/b1356d83fd199d934a6408bdc099ce6f

If solution is OK, next step is to test the upgrade with this solution.

akolotov · 2018-12-18T17:07:36Z

@patitonar I have started reviewing the gist files you prepared and will provide the feedback soon.

patitonar · 2018-12-18T17:14:53Z

@akolotov OK, let me know if there is any change we should perform.

I've tested the upgrade with the proposed changes, and everything worked as expected, no issues were found. State wasn't affected after the upgrades and bridge continue to work as normal.

I started with version v1.0 of contracts, then upgrade them to new version implemented on #109
and then upgrade them to the version proposed on this PR.

These were the steps for the upgrade of new roles:
- Call upgradeTo from proxy contract
- Call upgradeSecurityRoles(newBridgeOwner) on bridge contract
- Transfer other ownership to other accounts for testing

akolotov · 2018-12-18T19:46:32Z

@patitonar

Since we didn't touch initialize method in the previous upgrade in both contracts, my suggestion is to not touch it in this upgrade as well. It is also important if someone would like to decode the transaction with initialize invocation - the number and meaning of arguments will be the same.

Since we know in advance which owner will be used during the upgrade we could simplify the upgrade procedure if we use in HomeBridge

function upgradeFrom3To4() public  {
    require(owner() == address(0));
    setOwner(validatorContract().owner());
}

and in ForeignBridge

function upgradeFrom2To3() public  {
    require(owner() == address(0));
    setOwner(validatorContract().owner());
}

It will allow to call the methods in upgrateToAndCall.

As you see I changed the name of the call in order to have explicit correspondence between the version which is used in upgradeToAndCall method and the actions that is going to be made as part of the upgrade.

patitonar · 2018-12-19T14:15:16Z

@akolotov Updated the gist, please check the last changes.

Home: https://gist.github.com/patitonar/8caa3631e5a89b85ff3269a27c429813
Foreign: https://gist.github.com/patitonar/b1356d83fd199d934a6408bdc099ce6f

Should we run a new test with these updated Gist files?

akolotov · 2018-12-19T17:20:08Z

Yes, please. We cannot use untested changes for upgrade of the bridge in the production.

patitonar · 2018-12-19T19:28:52Z

@akolotov I followed the same steps mentioned on #116 (comment) except that this time I used upgradeToAndCall method to upgrade the contracts and the upgrade worked correctly, no issues were found.

akolotov · 2018-12-19T19:32:17Z

Thank you! Did you test that the bridge parameters can be changed by the old owner of validator contract after changing the owner of the validator contract to some another account?

patitonar · 2018-12-20T12:08:50Z

Yes. After the upgrade I tested the following steps for that case:

The new bridge owner can update parameters on the bridge contract
Validator's owner call transferOwnership to a new account
Bridge owner can still update parameters on the bridge contract
New Validator's owner can not update parameters on the bridge contract

akolotov · 2018-12-20T12:45:06Z

Thanks! I will initiate the process to upgrade the contracts of the POA bridge.

akolotov · 2019-01-08T11:03:09Z

The upgraded version of contracts was applied to the ETH mainnet and the POA core:

patitonar added 4 commits November 30, 2018 13:15

Add owner role to bridge contracts

f33c98f

Update deploy script with new roles

7614194

Add upgradeSecurityRoles

22bca33

Refactor onlyProxyOwner usage

8d31b9c

patitonar requested a review from akolotov December 4, 2018 18:53

ghost assigned patitonar Dec 4, 2018

ghost added in progress review labels Dec 4, 2018

igorbarinov removed the review label Dec 4, 2018

patitonar added review and removed in progress labels Dec 4, 2018

akolotov requested changes Dec 4, 2018

View reviewed changes

contracts/upgradeable_contracts/Validatable.sol Outdated Show resolved Hide resolved

Update upgradeSecurityRoles method

9846b97

ghost added the in progress label Dec 5, 2018

Remove upgradeSecurityRoles

f6ba43c

ghost removed the in progress label Dec 5, 2018

Fix validatable contract

8a1c6fb

ghost added the in progress label Dec 7, 2018

Refactor roles contract inheritance

1a42873

ghost removed the in progress label Dec 10, 2018

patitonar mentioned this pull request Dec 12, 2018

Update security roles on v2 contracts #117

Merged

Refactor onlyIfOwnerOfProxy modifier

76f7793

ghost added the in progress label Dec 17, 2018

ghost removed the in progress label Dec 17, 2018

Fix broken test

3a69300

ghost added the in progress label Dec 17, 2018

akolotov merged commit 65ac528 into develop-1.1 Jan 8, 2019

ghost removed the in progress label Jan 8, 2019

akolotov removed the review label Jan 8, 2019

akolotov mentioned this pull request Jan 8, 2019

Changes in security roles #111

Closed

akolotov deleted the 111-security-roles-1.1 branch January 8, 2019 11:16

akolotov mentioned this pull request Jan 8, 2019

Updates in the bridge contracts ABIs after recent security update poanetwork/poa-chain-spec#101

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update security roles on v1 contracts #116

Update security roles on v1 contracts #116

patitonar commented Dec 4, 2018 •

edited

Loading

akolotov commented Dec 4, 2018

patitonar commented Dec 5, 2018

akolotov commented Dec 5, 2018

patitonar commented Dec 5, 2018

patitonar commented Dec 5, 2018

akolotov commented Dec 5, 2018

patitonar commented Dec 7, 2018 •

edited

Loading

patitonar commented Dec 7, 2018

akolotov commented Dec 7, 2018

patitonar commented Dec 7, 2018

akolotov commented Dec 7, 2018

patitonar commented Dec 10, 2018

patitonar commented Dec 10, 2018

patitonar commented Dec 10, 2018

akolotov commented Dec 11, 2018

patitonar commented Dec 11, 2018 •

edited

Loading

patitonar commented Dec 17, 2018

akolotov commented Dec 18, 2018

patitonar commented Dec 18, 2018

akolotov commented Dec 18, 2018

patitonar commented Dec 19, 2018

akolotov commented Dec 19, 2018

patitonar commented Dec 19, 2018

akolotov commented Dec 19, 2018

patitonar commented Dec 20, 2018

akolotov commented Dec 20, 2018

akolotov commented Jan 8, 2019

Update security roles on v1 contracts #116

Update security roles on v1 contracts #116

Conversation

patitonar commented Dec 4, 2018 • edited Loading

akolotov commented Dec 4, 2018

patitonar commented Dec 5, 2018

akolotov commented Dec 5, 2018

patitonar commented Dec 5, 2018

patitonar commented Dec 5, 2018

akolotov commented Dec 5, 2018

patitonar commented Dec 7, 2018 • edited Loading

patitonar commented Dec 7, 2018

akolotov commented Dec 7, 2018

patitonar commented Dec 7, 2018

akolotov commented Dec 7, 2018

patitonar commented Dec 10, 2018

patitonar commented Dec 10, 2018

patitonar commented Dec 10, 2018

akolotov commented Dec 11, 2018

patitonar commented Dec 11, 2018 • edited Loading

patitonar commented Dec 17, 2018

akolotov commented Dec 18, 2018

patitonar commented Dec 18, 2018

akolotov commented Dec 18, 2018

patitonar commented Dec 19, 2018

akolotov commented Dec 19, 2018

patitonar commented Dec 19, 2018

akolotov commented Dec 19, 2018

patitonar commented Dec 20, 2018

akolotov commented Dec 20, 2018

akolotov commented Jan 8, 2019

patitonar commented Dec 4, 2018 •

edited

Loading

patitonar commented Dec 7, 2018 •

edited

Loading

patitonar commented Dec 11, 2018 •

edited

Loading