Update security roles on v2 contracts #117

patitonar · 2018-12-11T13:23:39Z

Closes #111

Since we are going to perform one upgrade on deployed contracts of ERC20-To-Native, this PR includes changes from #112.

To review specific changes from this PR, refer to commit abbde31

Once we agree on changes, these are the next steps:

Prepare gists files for upgrade
Test performing an upgrade of deployed contract
Update deploy script, .env.example and deploy/README.md file

contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol

akolotov · 2018-12-11T14:25:47Z

In general, I am OK with changes. Let's prepare a gist files for the erc-to-native mode in order to upgrade the ETH-xDai bridge.

patitonar · 2018-12-11T15:48:41Z

Here are the Gist files for each contract. I also described one method for each contract to be called when upgrading the contract to make the necessary updates for limits and new Owner role. Please check the values of the limits and update them as needed, also regarding the Owner, another option is to receive the new owner address as a parameter instead of using current validatorContract's owner.

Once we agree on the details of methods, I'll add them to the gist files.

Home Bridge Gist: https://gist.github.com/patitonar/39ac4d337420864bc912123f72bc41f7

// Method for Home Bridge
function upgradeToV220() public onlyProxyOwner {
  require(!boolStorage[keccak256(abi.encodePacked("isUpgradedToV220"))]);
  setOwner(validatorContract().owner());
  uintStorage[keccak256(abi.encodePacked("executionDailyLimit"))] = 1000000 ether;
  emit ExecutionDailyLimitChanged(3000000 ether);
  uintStorage[keccak256(abi.encodePacked("executionMaxPerTx"))] = 100000 ether;
  boolStorage[keccak256("isUpgradedToV220")] = true;
}

Foreign Bridge Gist: https://gist.github.com/patitonar/36a6e03c3ca13b912064b7742b5c3f5c

// Method for Foreign Bridge
function upgradeToV220() public onlyProxyOwner {
  require(!boolStorage[keccak256(abi.encodePacked("isUpgradedToV220"))]);
  setOwner(validatorContract().owner());
  uintStorage[keccak256(abi.encodePacked("maxPerTx"))] = 100000 ether;
  uintStorage[keccak256(abi.encodePacked("executionDailyLimit"))] = 1000000 ether;
  emit ExecutionDailyLimitChanged(3000000 ether);
  uintStorage[keccak256(abi.encodePacked("executionMaxPerTx"))] = 100000 ether;
  boolStorage[keccak256("isUpgradedToV220")] = true;
}

akolotov · 2018-12-11T16:18:13Z

Thanks!
1.

emit ExecutionDailyLimitChanged(3000000 ether);

Should be with the value 1000000 ether as well in both methods.

another option is to receive the new owner address as a parameter

Let's do in this way.

patitonar · 2018-12-11T16:49:31Z

Fixed the methods and added them into the Gist files. Here are the updated version:

// Method for Home Bridge
function upgradeToV220(address _newOwner) public onlyProxyOwner {
  require(!boolStorage[keccak256(abi.encodePacked("isUpgradedToV220"))]);
  setOwner(_newOwner);
  uintStorage[keccak256(abi.encodePacked("executionDailyLimit"))] = 1000000 ether;
  emit ExecutionDailyLimitChanged(1000000 ether);
  uintStorage[keccak256(abi.encodePacked("executionMaxPerTx"))] = 100000 ether;
  boolStorage[keccak256("isUpgradedToV220")] = true;
}

// Method for Foreign Bridge
function upgradeToV220(address _newOwner) public onlyProxyOwner {
    require(!boolStorage[keccak256(abi.encodePacked("isUpgradedToV220"))]);
    setOwner(_newOwner);
    uintStorage[keccak256(abi.encodePacked("maxPerTx"))] = 100000 ether;
    uintStorage[keccak256(abi.encodePacked("executionDailyLimit"))] = 1000000 ether;
    emit ExecutionDailyLimitChanged(1000000 ether);
    uintStorage[keccak256(abi.encodePacked("executionMaxPerTx"))] = 100000 ether;
    boolStorage[keccak256("isUpgradedToV220")] = true;
}

Next step is to test performing an upgrade of the contracts, test that we can send back tokens that exceed the limit and the new security roles.

patitonar · 2018-12-12T18:20:55Z

@akolotov I found an issue while testing. This same issue affects security roles upgrades also v1 contracts.

Previouly I described an issue regarding onlyProxyOwner modifier #116 (comment)

The solution implemented to that issue, was to change the order of inheritance between OwnedUpgradeabilityProxy and EternalStorage.
However, that solution is not correct because it caused the loss of the state of the contract while upgrading the contracts in the last test (because of changing the structure of the store). Also, accessing to the proxyOwner value was not correct since it was read from the contract (initialized when deploying new contract version and was the same address) and not from proxy contract.
So I think this change should be reverted.

The proxyOwner variable is stored in a separte variable on the proxy contract and not on the EternalStorage, so I think is not possible to access that value from Bridge contracts.
Here are some documentation about EternalStorageProxy strategy that we are using.
https://blog.zeppelinos.org/proxy-patterns/
https://github.com/zeppelinos/labs/tree/master/upgradeability_using_eternal_storage

Given this, I think a possible solution is to introduce an admin role for Bridge contracts (similar to Ownable), that could be set on initialize method too.
And also update the method upgradeToV220 to include setting the address for the admin role.
The bad side of this approach, is that if proxyOwner is changed on the proxy contract, this admin role would have to also be updated manually.

Does this solution sound good? Any other option you can think of?

akolotov · 2018-12-12T19:44:55Z

Given this, I think a possible solution is to introduce an admin role for Bridge contracts (similar to Ownable), that could be set on initialize method too.
And also update the method upgradeToV220 to include setting the address for the admin role.
The bad side of this approach, is that if proxyOwner is changed on the proxy contract, this admin role would have to also be updated manually.

Honestly, I don't like it. Let me think more about this.

akolotov · 2018-12-13T17:34:33Z

@patitonar I have an idea. I did not tried this but what address will be assigned to this if you try to access this variable from a method in the bridge contract? If this address is the same as address of the proxy contract (not the bridge contract implementation), you can try to invoke address(this).proxyOwner() or use the call functionality explicitly.

Could you try this?

akolotov

Good catch.

patitonar · 2018-12-17T14:34:54Z

I performed the test with the new roles and the new limits functionality. Everything worked as expected. Only one issue was found regarding functionality (setExecutionMaxPerTx) and was fixed.

Bridge admin // HomeBridge.owner()
- Can update parameter on bridge contract
- Can not update parameter on validators contract
- Can not upgrade bridge contract nor validators contract
- Can not claim tokens
Validators admin // BridgeValidators.owner()
- Can update parameter on validators contracts
- Can not update parameter on bridge contract
- Can not upgrade bridge contract nor validators contract
Upgradability admin // HomeBridgeEternalStorageProxy.proxyOwner()
- Can upgrade bridge contract
- Can claim tokens
- Can fix assets above limits
Upgradability admin // BridgeValidatorsEternalStorageProxy.proxyOwner()
- Can upgrade validators contract
State wasn't affected on contract upgrade.
About new limits functionality:
- Sent a Tx above daily execution limit
- Called fixAssetsAboveLimits(txHash, true) from Upgradability admin, which emitted UserRequestForSignature
- out of limit amount was reduced
- couldn't execute fixAssetsAboveLimits for the same txHash
- UserRequestForSignature was processed by Bridge

For the contract upgrade, I had to call upgradeTo and then call method upgradeToV220. I wasn't able to call upgradeToAndCall.

The reason is that upgradeToV220 can be executed only by proxyOwner. On upgradeToAndCall when executing the method upgradeToV220 the msg.sender is the address of the proxy contract and not the upgradability admin address.

https://github.com/poanetwork/poa-bridge-contracts/blob/1f61c695df23b2a8f79d96e138c6bb5d8cd5b592/contracts/upgradeability/OwnedUpgradeabilityProxy.sol#L69-L72

Are the upgrade steps OK or should we refactor upgradeToV220 method to split logic between setters of new limits (just public method to be called with upgradeToAndCall) and logic for setting the new bridge owner (public onlyIfOwnerOfProxy method to be called after upgrade) ?

akolotov · 2018-12-17T15:43:54Z

Only one issue was found regarding functionality (setExecutionMaxPerTx) and was fixed.

Was the gist updated with the fix?

patitonar · 2018-12-17T16:01:05Z

Was the gist updated with the fix?

Yes, both Gist files were updated. On Revisions section you can check the updates.

Home Bridge Gist: https://gist.github.com/patitonar/39ac4d337420864bc912123f72bc41f7
Foreign Bridge Gist: https://gist.github.com/patitonar/36a6e03c3ca13b912064b7742b5c3f5c

akolotov · 2018-12-17T16:05:13Z

Are the upgrade steps OK or should we refactor upgradeToV220 method to split logic between setters of new limits (just public method to be called with upgradeToAndCall) and logic for setting the new bridge owner (public onlyIfOwnerOfProxy method to be called after upgrade) ?

No, that's OK since the upgrade procedure is not trivial anyway:

Upgrade contracts
Set limits and owner for the bridge contracts on both sides
Set a new owner for the validator contract
Set a new upgrade admin for the validator contract

akolotov · 2018-12-17T16:12:40Z

So, let's finish changes for the contracts v1.x and distribute the changes to other modes of the bridge.

akolotov · 2018-12-19T10:26:41Z

@patitonar

Home Bridge Gist: https://gist.github.com/patitonar/39ac4d337420864bc912123f72bc41f7
Foreign Bridge Gist: https://gist.github.com/patitonar/36a6e03c3ca13b912064b7742b5c3f5c

During the upgrade we have found that

function upgradeToV220(address _newOwner) public onlyProxyOwner {

uses onlyProxyOwner that is not defined in the structure of used contracts. Most probably, onlyIfOwnerOfProxy should be used here.

Can you share why we did not catch this during the testing?

patitonar · 2018-12-19T13:12:41Z

@akolotov As you mentioned, correct version of upgradeToV220 should use onlyIfOwnerOfProxy.

Home:

    function upgradeToV220(address _newOwner) public onlyIfOwnerOfProxy {
        require(!boolStorage[keccak256(abi.encodePacked("isUpgradedToV220"))]);
        setOwner(_newOwner);
        uintStorage[keccak256(abi.encodePacked("executionDailyLimit"))] = 1000000 ether;
        emit ExecutionDailyLimitChanged(1000000 ether);
        uintStorage[keccak256(abi.encodePacked("executionMaxPerTx"))] = 100000 ether;
        boolStorage[keccak256("isUpgradedToV220")] = true;
    }

Foreign:

    function upgradeToV220(address _newOwner) public onlyIfOwnerOfProxy {
        require(!boolStorage[keccak256(abi.encodePacked("isUpgradedToV220"))]);
        setOwner(_newOwner);
        uintStorage[keccak256(abi.encodePacked("maxPerTx"))] = 100000 ether;
        uintStorage[keccak256(abi.encodePacked("executionDailyLimit"))] = 1000000 ether;
        emit ExecutionDailyLimitChanged(1000000 ether);
        uintStorage[keccak256(abi.encodePacked("executionMaxPerTx"))] = 100000 ether;
        boolStorage[keccak256("isUpgradedToV220")] = true;
    }

These correct versions of upgradeToV220 were used while I performed the tests.

What I think happened is that, after fixing the issue that was found during the test (on method setExecutionMaxPerTx) when updating the file that was going to be used to update the Gist file, I may have accidentally overwritten upgradeToV220 with the older version of the method, that used onlyProxyOwner.

To avoid this situation next time I think we should:

Push changes to Gist file first, before running the test.
Review carefully the updates performed to Gist files.
If any fix was made after a test, update the Gist file, and run a new test with it.

I have fixed the methods upgradeToV220 on the Gist Files. Let me know which should be the next steps here.

akolotov · 2018-12-19T17:17:49Z

Ok, thanks! We already deployed the contracts to the ETH mainnet and the xDai chain.

patitonar · 2018-12-21T19:56:48Z

@akolotov Applied changes to other modes, updated deploy scripts, .env.example and README file

akolotov · 2019-01-03T19:31:28Z

The upgraded version of contracts was applied to the ETH mainnet and the xDai chain:

patitonar added 13 commits November 23, 2018 16:37

Add opposite side limits WIP

5bd6f5d

Add total executed per day

42a87e6

Add fixAssetsAboveLimits method

aa2470a

Refactor execution limits names

7c07cde

Fix compile warnings

414e437

Add check on ErcToNative onFailedAffirmation method

12d0422

Refactor overdraw management

b7991b7

Refactor storage of out of limit values

6ad631b

Fix on onFailedAffirmation method

ab07c56

Update fixAssetsAboveLimits modifier to onlyProxyOwner

e4a3cf4

Update contract version

4612b7b

Fix OverdrawManagement inheritance

6069555

Update owner roles on bridge contracts

abbde31

ghost assigned patitonar Dec 11, 2018

ghost added in progress review labels Dec 11, 2018

patitonar requested a review from akolotov December 11, 2018 13:23

patitonar removed the in progress label Dec 11, 2018

Remove Validatable from OverdrawManagement

6c7320d

ghost added the in progress label Dec 11, 2018

akolotov reviewed Dec 11, 2018

View reviewed changes

contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol Show resolved Hide resolved

Update claimTokens methods modifier to onlyProxyOwner

adb81cf

ghost removed the in progress label Dec 11, 2018

akolotov approved these changes Dec 17, 2018

View reviewed changes

patitonar mentioned this pull request Dec 18, 2018

Extend limits functionality #112

Closed

patitonar added 2 commits December 20, 2018 15:58

Add OverdrawManagement to ERC20-TO-ERC20 contracts

8bf7c1a

Add execution limits to NATIVE-TO-ERC20 mode

06a9dcf

ghost removed the in progress label Dec 21, 2018

patitonar added 5 commits December 21, 2018 11:04

Update documentation with new roles

ba4b62c

Update Native-to-Erc20 deploy script

34f88f9

Update ERC20-To-Native deploy script

6bf96d3

Update ERC20-TO-ERC20 deploy script

e5d1522

Fix linter errors on deploy scripts

b4c1a8e

ghost added the in progress label Dec 21, 2018

patitonar removed the in progress label Dec 26, 2018

akolotov approved these changes Jan 3, 2019

View reviewed changes

akolotov merged commit 416a35d into develop Jan 3, 2019

ghost removed the review label Jan 3, 2019

This was referenced Jan 3, 2019

ERC-TO-NATIVE: extend limits functionality #110

Closed

Warn the user if a token transfer could exceed the limit on the token bridge omni/bridge-ui#178

Open

Changes in security roles #111

Closed

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

akolotov mentioned this pull request May 8, 2019

Monitor if the limit exceeds for 'erc20-to-native' and 'erc20-to-erc20' bridges omni/tokenbridge#30

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update security roles on v2 contracts #117

Update security roles on v2 contracts #117

patitonar commented Dec 11, 2018

akolotov commented Dec 11, 2018

patitonar commented Dec 11, 2018

akolotov commented Dec 11, 2018

patitonar commented Dec 11, 2018

patitonar commented Dec 12, 2018

akolotov commented Dec 12, 2018 •

edited

Loading

akolotov commented Dec 13, 2018 •

edited

Loading

akolotov left a comment

patitonar commented Dec 17, 2018

akolotov commented Dec 17, 2018

patitonar commented Dec 17, 2018

akolotov commented Dec 17, 2018

akolotov commented Dec 17, 2018

akolotov commented Dec 19, 2018

patitonar commented Dec 19, 2018

akolotov commented Dec 19, 2018

patitonar commented Dec 21, 2018

akolotov commented Jan 3, 2019

Update security roles on v2 contracts #117

Update security roles on v2 contracts #117

Conversation

patitonar commented Dec 11, 2018

akolotov commented Dec 11, 2018

patitonar commented Dec 11, 2018

akolotov commented Dec 11, 2018

patitonar commented Dec 11, 2018

patitonar commented Dec 12, 2018

akolotov commented Dec 12, 2018 • edited Loading

akolotov commented Dec 13, 2018 • edited Loading

akolotov left a comment

Choose a reason for hiding this comment

patitonar commented Dec 17, 2018

akolotov commented Dec 17, 2018

patitonar commented Dec 17, 2018

akolotov commented Dec 17, 2018

akolotov commented Dec 17, 2018

akolotov commented Dec 19, 2018

patitonar commented Dec 19, 2018

akolotov commented Dec 19, 2018

patitonar commented Dec 21, 2018

akolotov commented Jan 3, 2019

akolotov commented Dec 12, 2018 •

edited

Loading

akolotov commented Dec 13, 2018 •

edited

Loading