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.

Sign up for GitHub

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

AdminControlled: ownership transfer improvements #742

Merged
merged 5 commits into from
May 26, 2022
Merged

AdminControlled: ownership transfer improvements #742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
62c4d2d
AdminControlled: ownership transfer improvements
sept-en May 12, 2022
1a103d9
AdminControlled: remove redunant `adminOnly` mod
sept-en May 16, 2022
6336ce4
AdminControlled: ownership transfer improvements.
sept-en May 25, 2022
e0bead1
AdminControlled: tests fixes and improvements.
sept-en May 25, 2022
6cbecd6
Tests: remove redundant comment
sept-en May 26, 2022
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
10 changes: 5 additions & 5 deletions contracts/eth/nearbridge/contracts/AdminControlled.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ contract AdminControlled {
}
}

function verifyAdminAddress(address newAdmin) internal view onlyAdmin {
function verifyAdminAddress(address newAdmin) internal view {
require(newAdmin != admin, "Nominated admin is the same as the current");
// Zero address shouldn't be allowed as a security measure.
// If it's needed to remove the admin consider using address with all "1" digits.
Expand All @@ -54,10 +54,10 @@ contract AdminControlled {
nominatedAdmin = newAdmin;
}

function acceptAdmin(address newAdmin) public onlyAdmin {
verifyAdminAddress(newAdmin);
// Check that the new expected admin is the same as a nominated one
require(newAdmin == nominatedAdmin, "The provided admin address doesn't match the nominated one");
function acceptAdmin() public {
verifyAdminAddress(nominatedAdmin);
// Only nominated admin could accept its admin rights
require(msg.sender == nominatedAdmin, "Caller must be the nominated admin");

admin = nominatedAdmin;
// Explicitly set not allowed zero address for `nominatedAdmin` so it's impossible to accidentally change
Expand Down
66 changes: 48 additions & 18 deletions contracts/eth/nearbridge/test/NearBridge3.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,11 @@ describe('NearBridge with admin access', () => {
let adminAccount;
let userAccount1;
let userAccount2;
let newAdminAccount;

beforeEach(async () => {
ed25519 = await (await ethers.getContractFactory('Ed25519')).deploy();
[deployerAccount, userAccount1, userAccount2] = await ethers.getSigners();
[deployerAccount, userAccount1, userAccount2, newAdminAccount] = await ethers.getSigners();

// Make the deployer admin
adminAccount = deployerAccount;
Expand Down Expand Up @@ -196,7 +197,7 @@ describe('NearBridge with admin access', () => {

it('should nominate and accept the new admin', async () => {
const initialAdminAddress = await nearBridge.admin();
const newAdminAddress = '0x0123456789abcdefcafedeadbeefbea77a1de456';
const newAdminAddress = newAdminAccount.address;
expect(newAdminAddress)
.not
.equal(initialAdminAddress);
Expand All @@ -205,12 +206,14 @@ describe('NearBridge with admin access', () => {
expect(await nearBridge.admin())
.to
.equal(initialAdminAddress);
expect((await nearBridge.nominatedAdmin()).toLowerCase())
expect(await nearBridge.nominatedAdmin())
.to
.equal(newAdminAddress);

await nearBridge.acceptAdmin(newAdminAddress);
expect((await nearBridge.admin()).toLowerCase())
await nearBridge
.connect(newAdminAccount)
.acceptAdmin();
expect(await nearBridge.admin())
.to
.equal(newAdminAddress);
expect(await nearBridge.nominatedAdmin())
Expand All @@ -220,24 +223,27 @@ describe('NearBridge with admin access', () => {

it('should not accept the new admin twice', async () => {
const initialAdminAddress = await nearBridge.admin();
const newAdmin = userAccount2;
const newAdmin = newAdminAccount;
expect(newAdmin.address)
.not
.equal(initialAdminAddress);

await nearBridge.nominateAdmin(newAdmin.address);
await nearBridge.acceptAdmin(newAdmin.address);
expect((await nearBridge.admin()).toLowerCase())
await nearBridge
.connect(newAdmin)
.acceptAdmin();

expect(await nearBridge.admin())
.to
.equal(newAdmin.address.toLowerCase());
.equal(newAdmin.address);
await expect(
nearBridge
.connect(newAdmin)
.acceptAdmin(newAdmin.address)
.acceptAdmin()
)
.to
.be
.revertedWith('Nominated admin is the same as the current');
.revertedWith('Nominated admin shouldn\'t be zero address');
});

it('should not nominate the same admin', async () => {
Expand Down Expand Up @@ -268,32 +274,56 @@ describe('NearBridge with admin access', () => {
.equal(ethers.constants.AddressZero);
});

it('should not accept any other admin than the nominated one', async () => {
it('should not allow accepting admin any other account than the nominated one', async () => {
const initialAdminAddress = await nearBridge.admin();
const newAdminAddress = '0x0123456789abcdefcafedeadbeefbea77a1de456';
const newAdminAddress = newAdminAccount.address;
expect(newAdminAddress)
.not
.equal(initialAdminAddress);

await nearBridge.nominateAdmin(newAdminAddress);

const otherNewAdminAddress = '0xabcd0000abcd0000abcd0000abcd0000abcd0000';
await expect(nearBridge.acceptAdmin(otherNewAdminAddress))
// Should now allow the current admin to accept the new admin
await expect(nearBridge
.connect(adminAccount)
.acceptAdmin())
.to
.be
.revertedWith('The provided admin address doesn\'t match the nominated one');
.revertedWith('Caller must be the nominated admin');
});

it('should not accept the same admin', async () => {
const initialAdminAddress = await nearBridge.admin();
await expect(nearBridge.acceptAdmin(initialAdminAddress))
// Manually set the nominated admin to the same one
await nearBridge
.connect(adminAccount)
.adminSstore(1, initialAdminAddress);

// Verify that the nominated admin is indeed the same one (manually set)
expect(await nearBridge.nominatedAdmin())
.to
.equal(initialAdminAddress);

// Expect to fail in case the nominated admin is the same as the current one
await expect(nearBridge.acceptAdmin())
.to
.be
.revertedWith('Nominated admin is the same as the current');
});

it('should not accept the zero address admin', async () => {
await expect(nearBridge.acceptAdmin(ethers.constants.AddressZero))
// Manually set the nominated admin to the same one
await nearBridge
.connect(adminAccount)
.adminSstore(1, ethers.constants.AddressZero);

// Verify that the nominated admin is indeed the zero address (manually set)
expect(await nearBridge.nominatedAdmin())
.to
.equal(ethers.constants.AddressZero);

// Expect to fail in case the nominated admin is zero address
await expect(nearBridge.acceptAdmin())
.to
.be
.revertedWith('Nominated admin shouldn\'t be zero address');
Expand Down