Skip to content

Conversation

@patitonar
Copy link
Contributor

@patitonar patitonar commented Nov 20, 2019

Closes #318

@patitonar patitonar added the WIP Work in Progress label Nov 20, 2019
@patitonar patitonar removed the WIP Work in Progress label Nov 22, 2019
@patitonar patitonar marked this pull request as ready for review November 22, 2019 18:12
@patitonar patitonar self-assigned this Nov 22, 2019
@patitonar patitonar requested a review from akolotov November 22, 2019 18:13
Copy link
Collaborator

@akolotov akolotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit the current state of the review. I will continue with review of the tests.

@patitonar
Copy link
Contributor Author

Tested the implementation with MakerDAO contracts in Kovan testnet which are identical to the deployed in mainnet.

Sai Address: 0xc4375b7de8af5a38a93548eb8453a498222c4ff2
Dai Address: 0x4f96fe3b7a6cf9725f59d353f723c1bdb64ca6aa
ScdMcdMigration Address: 0x411b2faa662c8e3e5cf8f01dfdae0aee482ca7b0
Sai Top: 0x5f00393547561da3030ebf30e52f5dc0d5d3362c (from https://developer.makerdao.com/dai/1/api/)

Foreign Bridge Address: 0xb644F2c13D1e2410d224825991455fCBE95ACcd9

List of tx examples

@akolotov
Copy link
Collaborator

Tested the implementation with MakerDAO contracts in Kovan testnet which are identical to the deployed in mainnet.

thanks!

Copy link
Collaborator

@akolotov akolotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait for feedback from Quantstamp who is performing the security audit for the new tokenbridge contracts release.

Copy link
Collaborator

@akolotov akolotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patitonar we again forgot to increase version of the contracts after adding new functionality.

addressStorage[keccak256(abi.encodePacked("validatorsList", _prevValidator))] = _validator;
}

function isValidatorDuty(address _validator) external view returns (bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we increase a version in getBridgeValidatorsInterfacesVersion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should. Updated version to 2.3.0 be3cd33

return IScdMcdMigration(0xc73e0383F3Aff3215E6f04B0331D58CeCf0Ab849);
}

function swapTokens() public {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we increase the version returned by getBridgeInterfacesVersion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it makes sense. Updated to 2.5.0 c2c2c4c

}

function migrateToMCD(address _migrationContract) external onlyOwner {
function migrateToMCD() external onlyOwner {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As part of the security audit we have received a finding against this functionality and here is a recommendation: if the migration address is hardcoded or configurable, the migrateToMCD function does not need the onlyOwner modifier and anyone could invoke migrateToMCD, and thus trigger the migration.

So, let's remove the modifier onlyOwner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the method 251f949

@patitonar
Copy link
Contributor Author

Tested the upgrade with full changes following the steps mentioned here. Everything worked as expected and no issues were found.

Here are the new contract implementations:

function upgradeToV230() public {
    bytes32 upgradeStorage = 0x2e8a0420c66b3c19df2ac1cd4ffadb5637da547c6fd49388c7fe28dc8c13a8dd; // keccak256(abi.encodePacked('isUpgradedToV230'))
    require(!boolStorage[upgradeStorage]);
    address validator1 = 0x9ad83402C19Af24F76afa1930a2B2EEC2F47A8C5;
    address validator2 = 0x4D1c96B9A49C4469A0b720a22b74b034EDdFe051;
    address validator3 = 0x491FC792e78CDadd7D31446Bb7bDDef876a69AD6;
    address validator4 = 0xc073C8E5ED9Aa11CF6776C69b3e13b259Ba9F506;

    setNextValidator(F_ADDR, validator1);
    setNextValidator(validator1, validator2);
    setNextValidator(validator2, validator3);
    setNextValidator(validator3, validator4);
    setNextValidator(validator4, F_ADDR);
    boolStorage[upgradeStorage] = true;
}
function upgradeToV250() public {
    bytes32 upgradeStorage = 0x221181d03d3c423368b412861518cd98276a115f4c2d9594ab6697d90fc99c19; // keccak256(abi.encodePacked('isUpgradedToV250'))
    require(!boolStorage[upgradeStorage]);
    uintStorage[DAILY_LIMIT] = 100000 ether;
    emit DailyLimitChanged(100000 ether);
    uintStorage[MIN_PER_TX] = 5000000000000000;
    _setBridgeContractOnOtherSide(0x7301CFA0e1756B71869E93d4e4Dca5c7d0eb0AA6);
    boolStorage[upgradeStorage] = true;
}

I tested all steps using the strategy option 1 on Sokol and Kovan, using the set of contracts provided by makerDAO on Kovan. Except for step 9. Test Emergency Shutdown which was not possible to generate such a scenario.

I also tested upgrading in foreign side BridgeValidators and ForeignBridgeErcToNative with option 2 (using ganache-cli to fork the networks). I was able to deploy the new implementations and upgrade the contracts. I checked the state was not lost and new variables were correctly set. But I wasn't able to test the ui, oracle, and monitor in this case, because the forked local network generated by ganache was slow and sometimes it just stopped working when received a lot of requests, especially when trying to get events. Also, I wasn't able to deploy the new implementation for HomeBridgeErcToNative in the forked xDai, probably because of some issues with ganache-cli. I tested deploying the same contract directly in xDai and it worked correctly there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants