Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove proxyOwner method from proxy contract #198

Merged
merged 9 commits into from
Jun 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ build
flats
.node*
.idea
coverage
Empty file removed .node-xmlhttprequest-sync-27493
Empty file.
2 changes: 1 addition & 1 deletion contracts/interfaces/IOwnedUpgradeabilityProxy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@ pragma solidity 0.4.24;


interface IOwnedUpgradeabilityProxy {
function proxyOwner() public view returns (address);
function upgradeabilityOwner() public view returns (address);
}
20 changes: 6 additions & 14 deletions contracts/upgradeability/OwnedUpgradeabilityProxy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -26,26 +26,18 @@ contract OwnedUpgradeabilityProxy is UpgradeabilityOwnerStorage, UpgradeabilityP
/**
* @dev Throws if called by any account other than the owner.
*/
modifier onlyProxyOwner() {
require(msg.sender == proxyOwner());
modifier onlyUpgradeabilityOwner() {
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 {
function transferProxyOwnership(address newOwner) public onlyUpgradeabilityOwner {
require(newOwner != address(0));
emit ProxyOwnershipTransferred(proxyOwner(), newOwner);
emit ProxyOwnershipTransferred(upgradeabilityOwner(), newOwner);
setUpgradeabilityOwner(newOwner);
}

Expand All @@ -54,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);
}

Expand All @@ -66,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));
}
Expand Down
2 changes: 1 addition & 1 deletion contracts/upgradeability/UpgradeabilityOwnerStorage.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion contracts/upgradeable_contracts/BasicBridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ contract BasicBridge is EternalStorage, Validatable, Ownable, OwnedUpgradeabilit
return executionDailyLimit() >= nextLimit && _amount <= executionMaxPerTx();
}

function claimTokens(address _token, address _to) public onlyIfOwnerOfProxy {
function claimTokens(address _token, address _to) public onlyIfUpgradeabilityOwner {
require(_to != address(0));
if (_token == address(0)) {
_to.transfer(address(this).balance);
Expand Down
2 changes: 1 addition & 1 deletion contracts/upgradeable_contracts/OverdrawManagement.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ contract OverdrawManagement is EternalStorage, OwnedUpgradeability {

event UserRequestForSignature(address recipient, uint256 value);

function fixAssetsAboveLimits(bytes32 txHash, bool unlockOnForeign) external onlyIfOwnerOfProxy {
function fixAssetsAboveLimits(bytes32 txHash, bool unlockOnForeign) external onlyIfUpgradeabilityOwner {
require(!fixedAssets(txHash));
address recipient;
uint256 value;
Expand Down
11 changes: 3 additions & 8 deletions contracts/upgradeable_contracts/OwnedUpgradeability.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,9 @@ import "../interfaces/IOwnedUpgradeabilityProxy.sol";


contract OwnedUpgradeability {

function upgradeabilityAdmin() public view returns (address) {
return IOwnedUpgradeabilityProxy(this).proxyOwner();
}

// Avoid using onlyProxyOwner name to prevent issues with implementation from proxy contract
modifier onlyIfOwnerOfProxy() {
require(msg.sender == upgradeabilityAdmin());
// Avoid using onlyUpgradeabilityOwner name to prevent issues with implementation from proxy contract
modifier onlyIfUpgradeabilityOwner() {
require(msg.sender == IOwnedUpgradeabilityProxy(this).upgradeabilityOwner());
_;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ contract BasicForeignBridgeErcToErc is BasicBridge, BasicForeignBridge {
return bytes4(keccak256(abi.encodePacked("erc-to-erc-core")));
}

function claimTokens(address _token, address _to) public onlyIfOwnerOfProxy {
function claimTokens(address _token, address _to) public {
require(_token != address(erc20token()));
super.claimTokens(_token, _to);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ contract ForeignBridgeErcToNative is BasicBridge, BasicForeignBridge {
return bytes4(keccak256(abi.encodePacked("erc-to-native-core")));
}

function claimTokens(address _token, address _to) public onlyIfOwnerOfProxy {
function claimTokens(address _token, address _to) public {
require(_token != address(erc20token()));
super.claimTokens(_token, _to);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ contract ForeignBridgeNativeToErc is ERC677Receiver, BasicBridge, BasicForeignBr
return bytes4(keccak256(abi.encodePacked("native-to-erc-core")));
}

function claimTokensFromErc677(address _token, address _to) external onlyIfOwnerOfProxy {
function claimTokensFromErc677(address _token, address _to) external onlyIfUpgradeabilityOwner {
IBurnableMintableERC677Token(erc677token()).claimTokens(_token, _to);
}

Expand Down
13 changes: 0 additions & 13 deletions test/erc_to_erc/home_bridge.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 proxy owner', 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 upgradeabilityAdmin = await homeBridge.upgradeabilityAdmin()

upgradeabilityAdmin.should.be.equal(proxyOwner)
})
})

describe('#rewardableInitialize', async () => {
let homeFee
Expand Down
16 changes: 0 additions & 16 deletions test/erc_to_native/home_bridge.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 proxyOwner = await storageProxy.proxyOwner()
const data = homeContract.contract.methods
.initialize(
validatorContract.address,
Expand All @@ -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(proxyOwner)

const homeContractV2 = await HomeBridge.new()
await storageProxy.upgradeTo('2', homeContractV2.address).should.be.fulfilled
Expand All @@ -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(proxyOwner)
})
it('cant initialize with invalid arguments', async () => {
false.should.be.equal(await homeContract.isInitialized())
Expand Down Expand Up @@ -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 proxy owner', 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 upgradeabilityAdmin = await homeBridge.upgradeabilityAdmin()

upgradeabilityAdmin.should.be.equal(proxyOwner)
})
})

describe('#feeManager', async () => {
let homeBridge
Expand Down