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

(Fix) Deny repeatedly adding removed mining key #208

Merged
merged 10 commits into from
Jun 21, 2019
Merged

(Fix) Deny repeatedly adding removed mining key #208

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1999135
prevent repeated creation of mining key after removing
maxaleks Jun 17, 2019
d2a1195
fix existing tests
maxaleks Jun 17, 2019
0b963e4
add a test for repeatedly adding removed mining key
maxaleks Jun 17, 2019
4e0ecc2
make requested changes
maxaleks Jun 18, 2019
bad967d
deny swaping on removed key
maxaleks Jun 18, 2019
596a904
fix swapping keys and add a test for it
maxaleks Jun 19, 2019
175180b
return removed test and change it
maxaleks Jun 19, 2019
85de5c2
add more tests
maxaleks Jun 20, 2019
8b80098
add more tests for creating a ballot with removed mining key
maxaleks Jun 21, 2019
e253973
add missing key type
maxaleks Jun 21, 2019
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
17 changes: 17 additions & 0 deletions contracts/KeysManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ contract KeysManager is EternalStorage, IKeysManager {
bytes32 internal constant IS_MINING_ACTIVE = "isMiningActive";
bytes32 internal constant IS_PAYOUT_ACTIVE = "isPayoutActive";
bytes32 internal constant IS_VOTING_ACTIVE = "isVotingActive";
bytes32 internal constant HAS_MINING_KEY_BEEN_REMOVED = "hasMiningKeyBeenRemoved";
bytes32 internal constant MINING_KEY_BY_PAYOUT = "miningKeyByPayout";
bytes32 internal constant MINING_KEY_BY_VOTING = "miningKeyByVoting";
bytes32 internal constant MINING_KEY_HISTORY = "miningKeyHistory";
Expand Down Expand Up @@ -346,6 +347,12 @@ contract KeysManager is EternalStorage, IKeysManager {
];
}

function hasMiningKeyBeenRemoved(address _key) public view returns(bool) {
return boolStorage[
keccak256(abi.encode(HAS_MINING_KEY_BEEN_REMOVED, _key))
];
}

function getMiningKeyHistory(address _miningKey) public view returns(address) {
return miningKeyHistory(_miningKey);
}
Expand All @@ -357,6 +364,9 @@ contract KeysManager is EternalStorage, IKeysManager {
function addMiningKey(address _key) public onlyVotingToChangeKeys returns(bool) {
if (!_withinTotalLimit()) return false;
if (!initDisabled()) return false;
if (hasMiningKeyBeenRemoved(_key)) {
maxaleks marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
if (!IPoaNetworkConsensus(poaNetworkConsensus()).addValidator(_key, true)) {
return false;
}
maxaleks marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -402,6 +412,7 @@ contract KeysManager is EternalStorage, IKeysManager {
_setMiningKeyByPayout(payoutKey, address(0));
_clearMiningKey(_key);
_getValidatorMetadata().clearMetadata(_key);
_setHasMiningKeyBeenRemoved(_key);
maxaleks marked this conversation as resolved.
Show resolved Hide resolved
emit MiningKeyChanged(_key, "removed");
if (votingKey != address(0)) {
emit VotingKeyChanged(votingKey, _key, "removed");
Expand Down Expand Up @@ -599,6 +610,12 @@ contract KeysManager is EternalStorage, IKeysManager {
] = _success;
}

function _setHasMiningKeyBeenRemoved(address _key) internal {
maxaleks marked this conversation as resolved.
Show resolved Hide resolved
boolStorage[
keccak256(abi.encode(HAS_MINING_KEY_BEEN_REMOVED, _key))
] = true;
}

function _withinTotalLimit() private view returns(bool) {
IPoaNetworkConsensus poa = IPoaNetworkConsensus(poaNetworkConsensus());
return poa.getCurrentValidatorsLength() < maxLimitValidators();
Expand Down
1 change: 1 addition & 0 deletions contracts/VotingToChangeKeys.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ contract VotingToChangeKeys is VotingToChange, EnumKeyTypes {
address _newPayoutKey
) public returns(uint256) {
IKeysManager keysManager = _getKeysManager();
require(!keysManager.hasMiningKeyBeenRemoved(_newMiningKey));
maxaleks marked this conversation as resolved.
Show resolved Hide resolved
require(keysManager.miningKeyByVoting(_newVotingKey) == address(0));
require(keysManager.miningKeyByPayout(_newPayoutKey) == address(0));
require(_newVotingKey != _newMiningKey);
Expand Down
1 change: 1 addition & 0 deletions contracts/interfaces/IKeysManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ interface IKeysManager {
function isMiningActive(address) external view returns(bool);
function isVotingActive(address) external view returns(bool);
function isPayoutActive(address) external view returns(bool);
function hasMiningKeyBeenRemoved(address) external view returns(bool);
function getVotingByMining(address) external view returns(address);
function getPayoutByMining(address) external view returns(address);
function getTime() external view returns(uint256);
Expand Down
15 changes: 15 additions & 0 deletions test/keys_manager_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,21 @@ contract('KeysManager [all features]', function (accounts) {
logs[0].args.key.should.be.equal(accounts[2]);
logs[0].args.action.should.be.equal('added');
})
it('cannot add removed mining key', async () => {
maxaleks marked this conversation as resolved.
Show resolved Hide resolved
let data;
const key = accounts[3];

data = await keysManager.addMiningKey(key).should.be.fulfilled;
data.logs[0].event.should.be.equal('MiningKeyChanged');
data.logs[0].args.action.should.be.equal('added');

data = await keysManager.removeMiningKey(key).should.be.fulfilled;
data.logs[0].event.should.be.equal('MiningKeyChanged');
data.logs[0].args.action.should.be.equal('removed');

data = await keysManager.addMiningKey(key).should.be.fulfilled;
(data.logs[0] === undefined).should.be.equal(true);
})
})

describe('#addVotingKey', async () => {
Expand Down
17 changes: 16 additions & 1 deletion test/keys_manager_upgrade_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,21 @@ contract('KeysManager upgraded [all features]', function (accounts) {
logs[0].args.key.should.be.equal(accounts[2]);
logs[0].args.action.should.be.equal('added');
})
it('cannot add removed mining key', async () => {
let data;
const key = accounts[3];

data = await keysManager.addMiningKey(key).should.be.fulfilled;
data.logs[0].event.should.be.equal('MiningKeyChanged');
data.logs[0].args.action.should.be.equal('added');

data = await keysManager.removeMiningKey(key).should.be.fulfilled;
data.logs[0].event.should.be.equal('MiningKeyChanged');
data.logs[0].args.action.should.be.equal('removed');

data = await keysManager.addMiningKey(key).should.be.fulfilled;
(data.logs[0] === undefined).should.be.equal(true);
})
})

describe('#addVotingKey', async () => {
Expand Down Expand Up @@ -1009,4 +1024,4 @@ async function removePayoutKey(_miningKey, _shouldBeSuccessful, options) {
} else {
result.logs.length.should.be.equal(0);
}
}
}
7 changes: 4 additions & 3 deletions test/reward_by_time_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,10 @@ contract('RewardByTime [all features]', function (accounts) {
result.logs[0].args.rewards[3].toString().should.be.equal((emissionFundsAmount * 3).toString());
(await rewardByTime.lastTime.call()).should.be.bignumber.equal(100 + threshold * 7);
(await rewardByTime.keyIndex.call()).should.be.bignumber.equal(1);

await addMiningKey(miningKey2);
await addPayoutKey(payoutKey2, miningKey2);

const miningKey4 = '0x0000000000000000000000000000000000000001';
await addMiningKey(miningKey4);
await addPayoutKey(payoutKey2, miningKey4);
await poaNetworkConsensus.setSystemAddress(coinbase);
await poaNetworkConsensus.finalizeChange().should.be.fulfilled;
await poaNetworkConsensus.setSystemAddress('0xffffFFFfFFffffffffffffffFfFFFfffFFFfFFfE');
Expand Down
5 changes: 3 additions & 2 deletions test/reward_by_time_upgrade_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,9 @@ contract('RewardByTime upgraded [all features]', function (accounts) {
(await rewardByTime.lastTime.call()).should.be.bignumber.equal(100 + threshold * 7);
(await rewardByTime.keyIndex.call()).should.be.bignumber.equal(1);

await addMiningKey(miningKey2);
await addPayoutKey(payoutKey2, miningKey2);
const miningKey4 = '0x0000000000000000000000000000000000000001';
await addMiningKey(miningKey4);
await addPayoutKey(payoutKey2, miningKey4);
await poaNetworkConsensus.setSystemAddress(coinbase);
await poaNetworkConsensus.finalizeChange().should.be.fulfilled;
await poaNetworkConsensus.setSystemAddress('0xffffFFFfFFffffffffffffffFfFFFfffFFFfFFfE');
Expand Down