Skip to content

Commit

Permalink
Replace explicit module owner address with mediator owner
Browse files Browse the repository at this point in the history
  • Loading branch information
k1rill-fedoseev committed Mar 24, 2021
1 parent 51e57e5 commit 9427d0f
Show file tree
Hide file tree
Showing 10 changed files with 92 additions and 74 deletions.
5 changes: 5 additions & 0 deletions contracts/interfaces/IOwnable.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pragma solidity 0.7.5;

interface IOwnable {
function owner() external view returns (address);
}
30 changes: 0 additions & 30 deletions contracts/upgradeable_contracts/modules/MediatorOwnableModule.sol

This file was deleted.

44 changes: 44 additions & 0 deletions contracts/upgradeable_contracts/modules/OmnibridgeModule.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
pragma solidity 0.7.5;

import "@openzeppelin/contracts/utils/Address.sol";
import "../../interfaces/IOwnable.sol";

/**
* @title OmnibridgeModule
* @dev Common functionality for Omnibridge extension non-upgradeable module.
*/
abstract contract OmnibridgeModule is IOwnable {
IOwnable public mediator;

/**
* @dev Initializes this contract.
* @param _mediator address of the Omnibridge mediator contract that is allowed to perform additional actions on the particular module.
*/
constructor(IOwnable _mediator) {
mediator = _mediator;
}

/**
* @dev Throws if sender is not the owner of this contract.
*/
modifier onlyOwner {
require(msg.sender == owner());
_;
}

/**
* @dev Throws if sender is not the mediator.
*/
modifier onlyMediator {
require(msg.sender == address(mediator));
_;
}

/**
* Tells the contract owner address that is allowed to change configuration of this module.
* @return address of the owner account.
*/
function owner() public view override returns (address) {
return mediator.owner();
}
}
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
pragma solidity 0.7.5;

import "./TokenProxy.sol";
import "../OwnableModule.sol";
import "../OmnibridgeModule.sol";

/**
* @title TokenFactory
* @dev Factory contract for deployment of new TokenProxy contracts.
*/
contract TokenFactory is OwnableModule {
contract TokenFactory is OmnibridgeModule {
address public tokenImage;

/**
* @dev Initializes this contract
* @param _owner of this factory contract.
* @param _mediator address of the mediator contract used together with this token factory.
* @param _tokenImage address of the token image contract that should be used for creation of new tokens.
*/
constructor(address _owner, address _tokenImage) OwnableModule(_owner) {
constructor(IOwnable _mediator, address _tokenImage) OmnibridgeModule(_mediator) {
tokenImage = _tokenImage;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ pragma solidity 0.7.5;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/math/SafeMath.sol";
import "../MediatorOwnableModule.sol";
import "../OmnibridgeModule.sol";

/**
* @title OmnibridgeFeeManager
* @dev Implements the logic to distribute fees from the Omnibridge mediator contract operations.
* The fees are distributed in the form of ERC20/ERC677 tokens to the list of reward addresses.
*/
contract OmnibridgeFeeManager is MediatorOwnableModule {
contract OmnibridgeFeeManager is OmnibridgeModule {
using SafeMath for uint256;

// This is not a real fee value but a relative value used to calculate the fee percentage.
Expand All @@ -29,17 +29,15 @@ contract OmnibridgeFeeManager is MediatorOwnableModule {
/**
* @dev Stores the initial parameters of the fee manager.
* @param _mediator address of the mediator contract used together with this fee manager.
* @param _owner address of the contract owner.
* @param _rewardAddresses list of unique initial reward addresses, between whom fees will be distributed
* @param _fees array with initial fees for both bridge directions.
* [ 0 = homeToForeignFee, 1 = foreignToHomeFee ]
*/
constructor(
address _mediator,
address _owner,
IOwnable _mediator,
address[] memory _rewardAddresses,
uint256[2] memory _fees
) MediatorOwnableModule(_mediator, _owner) {
) OmnibridgeModule(_mediator) {
require(_rewardAddresses.length <= MAX_REWARD_ACCOUNTS);
_setFee(HOME_TO_FOREIGN_FEE, address(0), _fees[0]);
_setFee(FOREIGN_TO_HOME_FEE, address(0), _fees[1]);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
pragma solidity 0.7.5;

import "../OwnableModule.sol";
import "../OmnibridgeModule.sol";

/**
* @title MultiTokenForwardingRulesManager
* @dev Multi token mediator functionality for managing destination AMB lanes permissions.
*/
contract MultiTokenForwardingRulesManager is OwnableModule {
contract MultiTokenForwardingRulesManager is OmnibridgeModule {
address internal constant ANY_ADDRESS = 0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF;

// Forwarding rules mapping
Expand All @@ -15,7 +15,7 @@ contract MultiTokenForwardingRulesManager is OwnableModule {

event ForwardingRuleUpdated(address token, address sender, address receiver, int256 lane);

constructor(address _owner) OwnableModule(_owner) {}
constructor(IOwnable _mediator) OmnibridgeModule(_mediator) {}

/**
* @dev Tells the destination lane for a particular bridge operation by checking several wildcard forwarding rules.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import "../../BasicOmnibridge.sol";
* @title SelectorTokenGasLimitManager
* @dev Multi token mediator functionality for managing request gas limits.
*/
contract SelectorTokenGasLimitManager is OwnableModule {
contract SelectorTokenGasLimitManager is OmnibridgeModule {
IAMB public immutable bridge;

uint256 internal defaultGasLimit;
Expand All @@ -17,9 +17,9 @@ contract SelectorTokenGasLimitManager is OwnableModule {

constructor(
IAMB _bridge,
address _owner,
IOwnable _mediator,
uint256 _gasLimit
) OwnableModule(_owner) {
) OmnibridgeModule(_mediator) {
require(_gasLimit <= _bridge.maxGasPerTx());
bridge = _bridge;
defaultGasLimit = _gasLimit;
Expand Down
14 changes: 5 additions & 9 deletions deploy/src/omnibridge/foreign.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
const { web3Foreign, deploymentAddress } = require('../web3')
const { deployContract, upgradeProxy } = require('../deploymentUtils')
const { EternalStorageProxy, ForeignOmnibridge, PermittableToken, TokenFactory } = require('../loadContracts')
const {
FOREIGN_TOKEN_FACTORY,
FOREIGN_ERC677_TOKEN_IMAGE,
FOREIGN_BRIDGE_OWNER,
FOREIGN_TOKEN_NAME_SUFFIX,
} = require('../loadEnv')
const { FOREIGN_TOKEN_FACTORY, FOREIGN_ERC677_TOKEN_IMAGE, FOREIGN_TOKEN_NAME_SUFFIX } = require('../loadEnv')

async function deployForeign() {
let nonce = await web3Foreign.eth.getTransactionCount(deploymentAddress)
Expand All @@ -16,7 +11,8 @@ async function deployForeign() {
network: 'foreign',
nonce: nonce++,
})
console.log('[Foreign] Bridge Mediator Storage: ', foreignBridgeStorage.options.address)
const storageAddress = foreignBridgeStorage.options.address
console.log('[Foreign] Bridge Mediator Storage: ', storageAddress)

let tokenFactory = FOREIGN_TOKEN_FACTORY
if (!tokenFactory) {
Expand All @@ -34,7 +30,7 @@ async function deployForeign() {
console.log('\n[Foreign] Using existing ERC677 token image: ', foreignTokenImage)
}
console.log('\n[Foreign] Deploying new token factory')
const factory = await deployContract(TokenFactory, [FOREIGN_BRIDGE_OWNER, foreignTokenImage], {
const factory = await deployContract(TokenFactory, [storageAddress, foreignTokenImage], {
network: 'foreign',
nonce: nonce++,
})
Expand Down Expand Up @@ -63,7 +59,7 @@ async function deployForeign() {

console.log('\nForeign part of OMNIBRIDGE has been deployed\n')
return {
foreignBridgeMediator: { address: foreignBridgeStorage.options.address },
foreignBridgeMediator: { address: storageAddress },
tokenFactory: { address: tokenFactory },
}
}
Expand Down
18 changes: 9 additions & 9 deletions deploy/src/omnibridge/home.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ const {
HOME_ERC677_TOKEN_IMAGE,
HOME_TOKEN_FACTORY,
HOME_FORWARDING_RULES_MANAGER,
HOME_BRIDGE_OWNER,
HOME_REWARDABLE,
HOME_TRANSACTIONS_FEE,
FOREIGN_TRANSACTIONS_FEE,
Expand Down Expand Up @@ -33,7 +32,8 @@ async function deployHome() {
const homeBridgeStorage = await deployContract(EternalStorageProxy, [], {
nonce: nonce++,
})
console.log('[Home] Bridge Mediator Storage: ', homeBridgeStorage.options.address)
const storageAddress = homeBridgeStorage.options.address
console.log('[Home] Bridge Mediator Storage: ', storageAddress)

let tokenFactory = HOME_TOKEN_FACTORY
if (!tokenFactory) {
Expand All @@ -50,7 +50,7 @@ async function deployHome() {
console.log('\n[Home] Using existing ERC677 token image: ', homeTokenImage)
}
console.log('\n[Home] Deploying new token factory')
const factory = await deployContract(TokenFactory, [HOME_BRIDGE_OWNER, homeTokenImage], {
const factory = await deployContract(TokenFactory, [storageAddress, homeTokenImage], {
nonce: nonce++,
})
tokenFactory = factory.options.address
Expand All @@ -71,7 +71,7 @@ async function deployHome() {
`)
const manager = await deployContract(
OmnibridgeFeeManager,
[homeBridgeStorage.options.address, HOME_BRIDGE_OWNER, rewardList, [homeFeeInWei, foreignFeeInWei]],
[storageAddress, rewardList, [homeFeeInWei, foreignFeeInWei]],
{ nonce: nonce++ }
)
feeManager = manager.options.address
Expand All @@ -81,9 +81,9 @@ async function deployHome() {
let forwardingRulesManager = HOME_FORWARDING_RULES_MANAGER === false ? ZERO_ADDRESS : HOME_FORWARDING_RULES_MANAGER
if (forwardingRulesManager === '') {
console.log(`\n[Home] Deploying Forwarding Rules Manager contract with the following parameters:
OWNER: ${HOME_BRIDGE_OWNER}
MEDIATOR: ${storageAddress}
`)
const manager = await deployContract(MultiTokenForwardingRulesManager, [HOME_BRIDGE_OWNER], { nonce: nonce++ })
const manager = await deployContract(MultiTokenForwardingRulesManager, [storageAddress], { nonce: nonce++ })
forwardingRulesManager = manager.options.address
console.log('\n[Home] New Forwarding Rules Manager has been deployed: ', forwardingRulesManager)
} else {
Expand All @@ -92,11 +92,11 @@ async function deployHome() {

console.log(`\n[Home] Deploying gas limit manager contract with the following parameters:
HOME_AMB_BRIDGE: ${HOME_AMB_BRIDGE}
OWNER: ${HOME_BRIDGE_OWNER}
MEDIATOR: ${storageAddress}
`)
const gasLimitManager = await deployContract(
SelectorTokenGasLimitManager,
[HOME_AMB_BRIDGE, HOME_BRIDGE_OWNER, HOME_MEDIATOR_REQUEST_GAS_LIMIT],
[HOME_AMB_BRIDGE, storageAddress, HOME_MEDIATOR_REQUEST_GAS_LIMIT],
{ nonce: nonce++ }
)
console.log('\n[Home] New Gas Limit Manager has been deployed: ', gasLimitManager.options.address)
Expand All @@ -120,7 +120,7 @@ async function deployHome() {

console.log('\nHome part of OMNIBRIDGE has been deployed\n')
return {
homeBridgeMediator: { address: homeBridgeStorage.options.address },
homeBridgeMediator: { address: storageAddress },
tokenFactory: { address: tokenFactory },
feeManager: { address: feeManager },
gasLimitManager: { address: gasLimitManager.options.address },
Expand Down
25 changes: 15 additions & 10 deletions test/omnibridge/common.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ function runTests(accounts, isHome) {
PermittableToken = await requirePrecompiled('PermittableToken')

tokenImage = await PermittableToken.new('TEST', 'TST', 18, 1337)
tokenFactory = await TokenFactory.new(owner, tokenImage.address)
})

beforeEach(async () => {
Expand All @@ -142,6 +141,7 @@ function runTests(accounts, isHome) {
token = await ERC677BridgeToken.new('TEST', 'TST', 18)
currentDay = await contract.getCurrentDay()
tokenReceiver = await TokenReceiver.new()
tokenFactory = await TokenFactory.new(contract.address, tokenImage.address)
})

describe('getBridgeMode', () => {
Expand Down Expand Up @@ -293,6 +293,8 @@ function runTests(accounts, isHome) {
}
expect(await contract.owner()).to.be.equal(owner)
expect(await contract.tokenFactory()).to.be.equal(tokenFactory.address)
expect(await tokenFactory.owner()).to.be.equal(owner)
expect(await tokenFactory.mediator()).to.be.equal(contract.address)

expectEventInLogs(logs, 'ExecutionDailyLimitChanged', { token: ZERO_ADDRESS, newLimit: executionDailyLimit })
expectEventInLogs(logs, 'DailyLimitChanged', { token: ZERO_ADDRESS, newLimit: dailyLimit })
Expand Down Expand Up @@ -409,7 +411,7 @@ function runTests(accounts, isHome) {
})

it('should allow to change token factory', async () => {
const newTokenFactory = await TokenFactory.new(owner, tokenImage.address)
const newTokenFactory = await TokenFactory.new(contract.address, tokenImage.address)
await contract.setTokenFactory(owner, { from: owner }).should.be.rejected
await contract.setTokenFactory(newTokenFactory.address, { from: user }).should.be.rejected
await contract.setTokenFactory(newTokenFactory.address, { from: owner }).should.be.fulfilled
Expand All @@ -422,7 +424,11 @@ function runTests(accounts, isHome) {
describe('gas limit manager', () => {
let manager
beforeEach(async () => {
manager = await SelectorTokenGasLimitManager.new(ambBridgeContract.address, owner, 1000000)
manager = await SelectorTokenGasLimitManager.new(ambBridgeContract.address, contract.address, 1000000)
expect(await manager.owner()).to.be.equal(owner)
expect(await manager.mediator()).to.be.equal(contract.address)
expect(await manager.bridge()).to.be.equal(ambBridgeContract.address)
expect(await manager.methods['requestGasLimit()']()).to.be.bignumber.equal('1000000')
})

it('should allow to set new manager', async () => {
Expand All @@ -432,9 +438,6 @@ function runTests(accounts, isHome) {
await contract.setGasLimitManager(manager.address, { from: owner }).should.be.fulfilled

expect(await contract.gasLimitManager()).to.be.equal(manager.address)
expect(await manager.owner()).to.be.equal(owner)
expect(await manager.bridge()).to.be.equal(ambBridgeContract.address)
expect(await manager.methods['requestGasLimit()']()).to.be.bignumber.equal('1000000')
})

it('should allow to set request gas limit for specific selector', async () => {
Expand Down Expand Up @@ -1675,7 +1678,9 @@ function runTests(accounts, isHome) {
let feeManager
beforeEach(async () => {
await initialize().should.be.fulfilled
feeManager = await OmnibridgeFeeManager.new(contract.address, owner, [owner], [ether('0.02'), ether('0.01')])
feeManager = await OmnibridgeFeeManager.new(contract.address, [owner], [ether('0.02'), ether('0.01')])
expect(await feeManager.owner()).to.be.equal(owner)
expect(await feeManager.mediator()).to.be.equal(contract.address)
await contract.setFeeManager(feeManager.address, { from: owner }).should.be.fulfilled

const initialEvents = await getEvents(ambBridgeContract, { event: 'MockedEvent' })
Expand Down Expand Up @@ -2157,12 +2162,13 @@ function runTests(accounts, isHome) {
describe('oracle driven lane permissions', () => {
let manager
beforeEach(async () => {
manager = await MultiTokenForwardingRulesManager.new(owner)
await initialize().should.be.fulfilled
manager = await MultiTokenForwardingRulesManager.new(contract.address)
expect(await manager.owner()).to.be.equal(owner)
expect(await manager.mediator()).to.be.equal(contract.address)
})

it('should allow to update manager address', async () => {
await initialize().should.be.fulfilled
await contract.setForwardingRulesManager(manager.address, { from: user }).should.be.rejected
await contract.setForwardingRulesManager(manager.address, { from: owner }).should.be.fulfilled

Expand Down Expand Up @@ -2221,7 +2227,6 @@ function runTests(accounts, isHome) {
})

it('should send a message to the manual lane', async () => {
await initialize().should.be.fulfilled
await token.mint(user, ether('10'), { from: owner }).should.be.fulfilled
const args = [otherSideToken1, 'Test', 'TST', 18, user, value]
const data = contract.contract.methods.deployAndHandleBridgedTokens(...args).encodeABI()
Expand Down

0 comments on commit 9427d0f

Please sign in to comment.