Skip to content

Commit

Permalink
L1+L2: Use DEFAULT_ADMIN_ROLE in ControlledGateway + child contracts
Browse files Browse the repository at this point in the history 
code-423n4/2022-01-livepeer-findings#116
code-423n4/2022-01-livepeer-findings#63
code-423n4/2022-01-livepeer-findings#172
  • Loading branch information
@yondonfu
yondonfu committed Feb 1, 2022
1 parent fbca594 commit 46fa6b9
Show file tree
Hide file tree
Showing 6 changed files with 72 additions and 85 deletions.
8 changes: 3 additions & 5 deletions contracts/ControlledGateway.sol
View file
Expand Up @@ -7,11 +7,9 @@ import {AccessControl} from "@openzeppelin/contracts/access/AccessControl.sol";
/**
* @title ControlledGateway
* @notice Base Contract for both L1 and L2 LPT gateways. Provides AccessControl.
* Gateways can be paused by the governor to stop outgoing token migrations
* Gateways can be paused by the admin to stop outgoing token migrations
*/
contract ControlledGateway is AccessControl, Pausable {
bytes32 public constant GOVERNOR_ROLE = keccak256("GOVERNOR_ROLE");

address public immutable l1Lpt;
address public immutable l2Lpt;

Expand All @@ -22,11 +20,11 @@ contract ControlledGateway is AccessControl, Pausable {
l2Lpt = _l2Lpt;
}

function pause() external onlyRole(GOVERNOR_ROLE) {
function pause() external onlyRole(DEFAULT_ADMIN_ROLE) {
_pause();
}

function unpause() external onlyRole(GOVERNOR_ROLE) {
function unpause() external onlyRole(DEFAULT_ADMIN_ROLE) {
_unpause();
}
}
4 changes: 2 additions & 2 deletions contracts/L1/gateway/L1LPTGateway.sol
View file
Expand Up @@ -52,7 +52,7 @@ contract L1LPTGateway is IL1LPTGateway, ControlledGateway, L1ArbitrumMessenger {
*/
function setCounterpart(address _l2Counterpart)
external
onlyRole(GOVERNOR_ROLE)
onlyRole(DEFAULT_ADMIN_ROLE)
{
l2Counterpart = _l2Counterpart;
}
Expand All @@ -62,7 +62,7 @@ contract L1LPTGateway is IL1LPTGateway, ControlledGateway, L1ArbitrumMessenger {
* @dev Only address with the governor role is allowed to change the value of minter
* @param _minter L1 Address of minter
*/
function setMinter(address _minter) external onlyRole(GOVERNOR_ROLE) {
function setMinter(address _minter) external onlyRole(DEFAULT_ADMIN_ROLE) {
minter = _minter;
}

Expand Down
2 changes: 1 addition & 1 deletion contracts/L2/gateway/L2LPTGateway.sol
View file
Expand Up @@ -49,7 +49,7 @@ contract L2LPTGateway is IL2LPTGateway, ControlledGateway, L2ArbitrumMessenger {
*/
function setCounterpart(address _l1Counterpart)
external
onlyRole(GOVERNOR_ROLE)
onlyRole(DEFAULT_ADMIN_ROLE)
{
l1Counterpart = _l1Counterpart;
}
Expand Down
45 changes: 21 additions & 24 deletions test/unit/L1/l1LPTGateway.test.ts
View file
Expand Up @@ -22,7 +22,7 @@ describe('L1 LPT Gateway', function() {
let owner: SignerWithAddress;
let sender: SignerWithAddress;
let receiver: SignerWithAddress;
let governor: SignerWithAddress;
let admin: SignerWithAddress;

// mocks
let inboxMock: FakeContract;
Expand All @@ -40,17 +40,14 @@ describe('L1 LPT Gateway', function() {
const initialTotalL1Supply = 3000;
const depositAmount = 100;

const GOVERNOR_ROLE = ethers.utils.solidityKeccak256(
['string'],
['GOVERNOR_ROLE'],
);
const DEFAULT_ADMIN_ROLE = ethers.constants.HashZero;

beforeEach(async function() {
[
owner,
sender,
receiver,
governor,
admin,
mockOutboxEOA,
mockInboxEOA,
mockBridgeEOA,
Expand Down Expand Up @@ -98,9 +95,9 @@ describe('L1 LPT Gateway', function() {
await token.transfer(sender.address, initialTotalL1Supply);
await token.connect(sender).approve(l1Gateway.address, depositAmount);

await l1Gateway.grantRole(GOVERNOR_ROLE, governor.address);
await l1Gateway.connect(governor).setCounterpart(mockL2GatewayEOA.address);
await l1Gateway.connect(governor).setMinter(mockMinterEOA.address);
await l1Gateway.grantRole(DEFAULT_ADMIN_ROLE, admin.address);
await l1Gateway.connect(admin).setCounterpart(mockL2GatewayEOA.address);
await l1Gateway.connect(admin).setMinter(mockMinterEOA.address);

inboxMock = await smock.fake('IInbox', {
address: mockInboxEOA.address,
Expand Down Expand Up @@ -162,19 +159,19 @@ describe('L1 LPT Gateway', function() {
ethers.utils.solidityKeccak256(['string'], ['newAddress']).slice(0, 42),
);

describe('caller not governor', () => {
describe('caller not admin', () => {
it('should fail to change counterpart address', async function() {
const tx = l1Gateway.connect(owner).setCounterpart(newAddress);
expect(tx).to.be.revertedWith(
const tx = l1Gateway.connect(sender).setCounterpart(newAddress);
await expect(tx).to.be.revertedWith(
// eslint-disable-next-line
`AccessControl: account ${owner.address.toLocaleLowerCase()} is missing role ${GOVERNOR_ROLE}`
`AccessControl: account ${sender.address.toLocaleLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`
);
});
});

describe('caller is governor', () => {
describe('caller is admin', () => {
it('should change counterpart address', async function() {
await l1Gateway.connect(governor).setCounterpart(newAddress);
await l1Gateway.connect(admin).setCounterpart(newAddress);
const counterpart = await l1Gateway.counterpartGateway();
expect(counterpart).to.equal(newAddress);
});
Expand All @@ -186,19 +183,19 @@ describe('L1 LPT Gateway', function() {
ethers.utils.solidityKeccak256(['string'], ['newAddress']).slice(0, 42),
);

describe('caller not governor', () => {
describe('caller not admin', () => {
it('should fail to change minter address', async function() {
const tx = l1Gateway.connect(owner).setMinter(newAddress);
expect(tx).to.be.revertedWith(
const tx = l1Gateway.connect(sender).setMinter(newAddress);
await expect(tx).to.be.revertedWith(
// eslint-disable-next-line
`AccessControl: account ${owner.address.toLocaleLowerCase()} is missing role ${GOVERNOR_ROLE}`
`AccessControl: account ${sender.address.toLocaleLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`
);
});
});

describe('caller is governor', () => {
describe('caller is admin', () => {
it('should change counterpart address', async function() {
await l1Gateway.connect(governor).setMinter(newAddress);
await l1Gateway.connect(admin).setMinter(newAddress);
const minter = await l1Gateway.minter();
expect(minter).to.equal(newAddress);
});
Expand All @@ -225,12 +222,12 @@ describe('L1 LPT Gateway', function() {
);

beforeEach(async function() {
await l1Gateway.connect(governor).unpause();
await l1Gateway.connect(admin).unpause();
});

describe('when gateway is paused', async function() {
beforeEach(async function() {
await l1Gateway.connect(governor).pause();
await l1Gateway.connect(admin).pause();
});

it('should fail to tranfer', async function() {
Expand Down Expand Up @@ -700,7 +697,7 @@ describe('L1 LPT Gateway', function() {

describe('when gateway is not paused', () => {
beforeEach(async function() {
await l1Gateway.connect(governor).unpause();
await l1Gateway.connect(admin).unpause();
});

it('sends funds from the escrow', async () => {
Expand Down
31 changes: 14 additions & 17 deletions test/unit/L2/l2LPTGateway.test.ts
View file
Expand Up @@ -20,7 +20,7 @@ describe('L2 Gateway', function() {
let l2Gateway: L2LPTGateway;
let sender: SignerWithAddress;
let receiver: SignerWithAddress;
let governor: SignerWithAddress;
let admin: SignerWithAddress;

// mocks
let arbSysMock: FakeContract;
Expand All @@ -36,17 +36,14 @@ describe('L2 Gateway', function() {
['BURNER_ROLE'],
);

const GOVERNOR_ROLE = ethers.utils.solidityKeccak256(
['string'],
['GOVERNOR_ROLE'],
);
const DEFAULT_ADMIN_ROLE = ethers.constants.HashZero;

beforeEach(async function() {
[
owner,
sender,
receiver,
governor,
admin,
mockL2RouterEOA,
mockL1GatewayEOA,
mockL1LptEOA,
Expand Down Expand Up @@ -75,8 +72,8 @@ describe('L2 Gateway', function() {
l2Gateway.address,
);

await l2Gateway.grantRole(GOVERNOR_ROLE, governor.address);
await l2Gateway.connect(governor).setCounterpart(mockL1GatewayEOA.address);
await l2Gateway.grantRole(DEFAULT_ADMIN_ROLE, admin.address);
await l2Gateway.connect(admin).setCounterpart(mockL1GatewayEOA.address);

await token.grantRole(BURNER_ROLE, l2Gateway.address);

Expand Down Expand Up @@ -143,19 +140,19 @@ describe('L2 Gateway', function() {
ethers.utils.solidityKeccak256(['string'], ['newAddress']).slice(0, 42),
);

describe('caller not governor', () => {
describe('caller not admin', () => {
it('should fail to change counterpart address', async function() {
const tx = l2Gateway.connect(owner).setCounterpart(newAddress);
expect(tx).to.be.revertedWith(
const tx = l2Gateway.connect(sender).setCounterpart(newAddress);
await expect(tx).to.be.revertedWith(
// eslint-disable-next-line
`AccessControl: account ${owner.address.toLocaleLowerCase()} is missing role ${GOVERNOR_ROLE}`
`AccessControl: account ${sender.address.toLocaleLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`
);
});
});

describe('caller is governor', () => {
describe('caller is admin', () => {
it('should change counterpart address', async function() {
await l2Gateway.connect(governor).setCounterpart(newAddress);
await l2Gateway.connect(admin).setCounterpart(newAddress);
const counterpart = await l2Gateway.counterpartGateway();
expect(counterpart).to.equal(newAddress);
});
Expand All @@ -171,7 +168,7 @@ describe('L2 Gateway', function() {

describe('when gateway is not paused', () => {
beforeEach(async function() {
await l2Gateway.connect(governor).unpause();
await l2Gateway.connect(admin).unpause();
});

describe('caller is not l1 gateway router (aliased)', () => {
Expand Down Expand Up @@ -347,12 +344,12 @@ describe('L2 Gateway', function() {
owner.address,
);
await token.connect(owner).mint(sender.address, initialTotalL2Supply);
await l2Gateway.connect(governor).unpause();
await l2Gateway.connect(admin).unpause();
});

describe('when gateway is paused', async function() {
beforeEach(async function() {
await l2Gateway.connect(governor).pause();
await l2Gateway.connect(admin).pause();
});

it('should fail to tranfer', async () => {
Expand Down

0 comments on commit 46fa6b9

Please sign in to comment.