Merge pull request #175 from varasev/security-audit

(Refactor) Remove `this` and other code improvements

contracts/BallotsStorage.sol

@@ -80,8 +80,8 @@ contract BallotsStorage is EternalStorage, EnumBallotTypes, EnumKeyTypes, EnumTh
     ) public onlyOwner {
         require(!initDisabled());
         require(_thresholds.length == uint256(ThresholdTypes.MetadataChange));
-        //require(_thresholds.length < 255);
-        for (uint256 thresholdType = uint256(ThresholdTypes.Keys); thresholdType <= _thresholds.length; thresholdType++) {
+        uint256 thresholdType = uint256(ThresholdTypes.Keys);
+        for (; thresholdType <= _thresholds.length; thresholdType++) {
             uint256 thresholdValue = _thresholds[thresholdType - uint256(ThresholdTypes.Keys)];
             if (!_setThreshold(thresholdValue, thresholdType)) {
                 revert();

contracts/EmissionFunds.sol

@@ -9,21 +9,18 @@ contract EmissionFunds is IEmissionFunds {
     event FundsSentTo(
         address indexed receiver,
         address caller,
-        address indexed callerOrigin,
         uint256 amount,
         bool indexed success
     );
 
     event FundsBurnt(
         address caller,
-        address indexed callerOrigin,
         uint256 amount,
         bool indexed success
     );
 
     event FundsFrozen(
         address caller,
-        address indexed callerOrigin,
         uint256 amount
     );
 
@@ -48,7 +45,7 @@ contract EmissionFunds is IEmissionFunds {
         // using `send` instead of `transfer` to avoid revert on failure
         bool success = _receiver.send(_amount);
         // solhint-disable avoid-tx-origin
-        emit FundsSentTo(_receiver, msg.sender, tx.origin, _amount, success);
+        emit FundsSentTo(_receiver, msg.sender, _amount, success);
         // solhint-enable avoid-tx-origin
         return success;
     }
@@ -61,7 +58,7 @@ contract EmissionFunds is IEmissionFunds {
         // using `send` instead of `transfer` to avoid revert on failure
         bool success = address(0).send(_amount);
         // solhint-disable avoid-tx-origin
-        emit FundsBurnt(msg.sender, tx.origin, _amount, success);
+        emit FundsBurnt(msg.sender, _amount, success);
         // solhint-enable avoid-tx-origin
         return success;
     }
@@ -71,7 +68,7 @@ contract EmissionFunds is IEmissionFunds {
         onlyVotingToManageEmissionFunds
     {
         // solhint-disable avoid-tx-origin
-        emit FundsFrozen(msg.sender, tx.origin, _amount);
+        emit FundsFrozen(msg.sender, _amount);
         // solhint-enable avoid-tx-origin
     }
 }

contracts/KeysManager.sol

@@ -166,17 +166,17 @@ contract KeysManager is EternalStorage, IKeysManager {
     }
 
     function validatorKeys(address _miningKey) public view returns(
-        address votingKey,
-        address payoutKey,
-        bool isMiningActive,
-        bool isVotingActive,
-        bool isPayoutActive
+        address validatorVotingKey,
+        address validatorPayoutKey,
+        bool isValidatorMiningActive,
+        bool isValidatorVotingActive,
+        bool isValidatorPayoutActive
     ) {
-        votingKey = this.getVotingByMining(_miningKey);
-        payoutKey = this.getPayoutByMining(_miningKey);
-        isMiningActive = this.isMiningActive(_miningKey);
-        isVotingActive = this.isVotingActiveByMiningKey(_miningKey);
-        isPayoutActive = this.isPayoutActive(_miningKey);
+        validatorVotingKey = getVotingByMining(_miningKey);
+        validatorPayoutKey = getPayoutByMining(_miningKey);
+        isValidatorMiningActive = isMiningActive(_miningKey);
+        isValidatorVotingActive = isVotingActiveByMiningKey(_miningKey);
+        isValidatorPayoutActive = isPayoutActive(_miningKey);
     }
 
     function initDisabled() public view returns(bool) {

contracts/RewardByBlock.sol

@@ -3,7 +3,6 @@ pragma solidity ^0.4.24;
 import "./interfaces/IRewardByBlock.sol";
 import "./interfaces/IKeysManager.sol";
 import "./interfaces/IProxyStorage.sol";
-import "./interfaces/IPoaNetworkConsensus.sol";
 import "./eternal-storage/EternalStorage.sol";
 import "./libs/SafeMath.sol";
 

contracts/ValidatorMetadata.sol

@@ -525,6 +525,7 @@ contract ValidatorMetadata is EternalStorage, EnumThresholdTypes, IValidatorMeta
         address _miningKey,
         string _fullAddress
     ) private {
+        require(bytes(_fullAddress).length <= 200);
         stringStorage[keccak256(abi.encode(
             _storeName(_pending),
             _miningKey,

contracts/VotingToChangeMinThreshold.sol

@@ -37,7 +37,7 @@ contract VotingToChangeMinThreshold is VotingToChange {
         address creator,
         string memo,
         bool canBeFinalizedNow,
-        bool hasAlreadyVoted
+        bool alreadyVoted
     ) {
         startTime = _getStartTime(_id);
         endTime = _getEndTime(_id);
@@ -48,7 +48,7 @@ contract VotingToChangeMinThreshold is VotingToChange {
         creator = _getCreator(_id);
         memo = _getMemo(_id);
         canBeFinalizedNow = _canBeFinalizedNow(_id);
-        hasAlreadyVoted = this.hasAlreadyVoted(_id, _votingKey);
+        alreadyVoted = hasAlreadyVoted(_id, _votingKey);
     }
 
     function init(

contracts/VotingToChangeProxyAddress.sol

@@ -38,7 +38,7 @@ contract VotingToChangeProxyAddress is VotingToChange {
         address creator,
         string memo,
         bool canBeFinalizedNow,
-        bool hasAlreadyVoted
+        bool alreadyVoted
     ) {
         startTime = _getStartTime(_id);
         endTime = _getEndTime(_id);
@@ -50,7 +50,7 @@ contract VotingToChangeProxyAddress is VotingToChange {
         creator = _getCreator(_id);
         memo = _getMemo(_id);
         canBeFinalizedNow = _canBeFinalizedNow(_id);
-        hasAlreadyVoted = this.hasAlreadyVoted(_id, _votingKey);
+        alreadyVoted = hasAlreadyVoted(_id, _votingKey);
     }
 
     function migrateBasicOne(

contracts/VotingToManageEmissionFunds.sol

@@ -18,8 +18,8 @@ contract VotingToManageEmissionFunds is VotingTo {
     bytes32 internal constant EMISSION_RELEASE_TIME =
         keccak256("emissionReleaseTime");
 
-    bytes32 internal constant PREVIOUS_BALLOT_FINALIZED =
-        keccak256("previousBallotFinalized");
+    bytes32 internal constant NO_ACTIVE_BALLOT_EXISTS =
+        keccak256("noActiveBallotExists");
 
     bytes32 internal constant AMOUNT = "amount";
     bytes32 internal constant BURN_VOTES = "burnVotes";
@@ -50,7 +50,7 @@ contract VotingToManageEmissionFunds is VotingTo {
         if (isActive(_id)) return false;
         if (_getIsCanceled(_id)) return false;
         if (_getIsFinalized(_id)) return false;
-        if (previousBallotFinalized()) return false;
+        if (noActiveBallotExists()) return false;
         if (_withinCancelingThreshold(_id)) return false;
         return true;
     }
@@ -63,7 +63,7 @@ contract VotingToManageEmissionFunds is VotingTo {
         require(!_getIsCanceled(ballotId));
         require(!_getIsFinalized(ballotId));
         _setIsCanceled(ballotId, true);
-        _setPreviousBallotFinalized(true);
+        _setNoActiveBallotExists(true);
         _setEmissionReleaseTime(_getEmissionReleaseTimeSnapshot(ballotId));
         emit BallotCanceled(ballotId, msg.sender);
     }
@@ -79,11 +79,10 @@ contract VotingToManageEmissionFunds is VotingTo {
         require(_endTime > _startTime && _startTime > currentTime);
         uint256 releaseTimeSnapshot = emissionReleaseTime();
         uint256 releaseTime = refreshEmissionReleaseTime();
-        require(_startTime > releaseTime);
-        require(currentTime > releaseTime);
+        require(currentTime >= releaseTime);
         require(_endTime.sub(releaseTime) <= distributionThreshold());
         require(_receiver != address(0));
-        require(previousBallotFinalized());
+        require(noActiveBallotExists());
         uint256 ballotId = _createBallot(
             uint256(BallotTypes.ManageEmissionFunds),
             _startTime,
@@ -97,7 +96,7 @@ contract VotingToManageEmissionFunds is VotingTo {
         _setFreezeVotes(ballotId, 0);
         _setReceiver(ballotId, _receiver);
         _setAmount(ballotId, emissionFunds().balance);
-        _setPreviousBallotFinalized(false);
+        _setNoActiveBallotExists(false);
         _setCreationTime(ballotId, currentTime);
         _setEmissionReleaseTimeSnapshot(ballotId, releaseTimeSnapshot);
         _setIsCanceled(ballotId, false);
@@ -168,16 +167,16 @@ contract VotingToManageEmissionFunds is VotingTo {
         require(_emissionReleaseThreshold > 0);
         require(_distributionThreshold > ballotCancelingThreshold());
         require(_emissionReleaseThreshold > _distributionThreshold);
-        _setPreviousBallotFinalized(true);
+        _setNoActiveBallotExists(true);
         _setEmissionReleaseTime(_emissionReleaseTime);
         addressStorage[EMISSION_FUNDS] = _emissionFunds;
         uintStorage[EMISSION_RELEASE_THRESHOLD] = _emissionReleaseThreshold;
         uintStorage[DISTRIBUTION_THRESHOLD] = _distributionThreshold;
         boolStorage[INIT_DISABLED] = true;
     }
 
-    function previousBallotFinalized() public view returns(bool) {
-        return boolStorage[PREVIOUS_BALLOT_FINALIZED];
+    function noActiveBallotExists() public view returns(bool) {
+        return boolStorage[NO_ACTIVE_BALLOT_EXISTS];
     }
 
     function refreshEmissionReleaseTime() public returns(uint256) {
@@ -229,7 +228,7 @@ contract VotingToManageEmissionFunds is VotingTo {
         uint256 amount = _getAmount(_id);
 
         _setIsFinalized(_id, true);
-        _setPreviousBallotFinalized(true);
+        _setNoActiveBallotExists(true);
         _setEmissionReleaseTime(
             emissionReleaseTime().add(emissionReleaseThreshold())
         );
@@ -380,8 +379,8 @@ contract VotingToManageEmissionFunds is VotingTo {
         ] = _canceled;
     }
 
-    function _setPreviousBallotFinalized(bool _finalized) private {
-        boolStorage[PREVIOUS_BALLOT_FINALIZED] = _finalized;
+    function _setNoActiveBallotExists(bool _finalized) private {
+        boolStorage[NO_ACTIVE_BALLOT_EXISTS] = _finalized;
     }
 
     function _setReceiver(uint256 _ballotId, address _value) private {

contracts/interfaces/IBallotsStorage.sol

@@ -12,6 +12,7 @@ interface IBallotsStorage {
     function metadataChangeConfirmationsLimit() external pure returns(uint256);
 }
 
+
 interface IBallotsStoragePrev {
     function getBallotThreshold(uint8) external view returns(uint256);
 }

scripts/migrate/migrateAll.js

@@ -226,7 +226,7 @@ async function main() {
 			await votingToManageEmissionFundsInstance.methods.initDisabled().call()
 		);
 		true.should.be.equal(
-			await votingToManageEmissionFundsInstance.methods.previousBallotFinalized().call()
+			await votingToManageEmissionFundsInstance.methods.noActiveBallotExists().call()
 		);
 		EthereumUtil.toChecksumAddress(votingToManageEmissionFundsAddress).should.be.equal(
 			EthereumUtil.toChecksumAddress(await emissionFundsInstance.methods.votingToManageEmissionFunds().call())

scripts/migrate/migrateKeys.js

@@ -174,9 +174,16 @@ async function migrateAndCheck(privateKey) {
 			console.log(`  Check mining key ${miningKey}...`);
 
 			try {
-				(await keysManagerOldInstance.methods.validatorKeys(miningKey).call()).should.be.deep.equal(
-					await keysManagerNewInstance.methods.validatorKeys(miningKey).call()
-				);
+				const validatorOldKeys = await keysManagerOldInstance.methods.validatorKeys(miningKey).call();
+				const validatorNewKeys = await keysManagerNewInstance.methods.validatorKeys(miningKey).call();
+				validatorOldKeys[0].should.be.equal(validatorNewKeys[0]);
+				validatorOldKeys[1].should.be.equal(validatorNewKeys[1]);
+				validatorOldKeys[2].should.be.equal(validatorNewKeys[2]);
+				validatorOldKeys[3].should.be.equal(validatorNewKeys[3]);
+				validatorOldKeys[4].should.be.equal(validatorNewKeys[4]);
+				//(await keysManagerOldInstance.methods.validatorKeys(miningKey).call()).should.be.deep.equal(
+				//	await keysManagerNewInstance.methods.validatorKeys(miningKey).call()
+				//);
 				const votingKey = await keysManagerOldInstance.methods.getVotingByMining(miningKey).call();
 				votingKey.should.be.equal(
 					await keysManagerNewInstance.methods.getVotingByMining(miningKey).call()

test/metadata_test.js

@@ -107,6 +107,13 @@ contract('ValidatorMetadata [all features]', function (accounts) {
       logs[0].event.should.be.equal('MetadataCreated');
       logs[0].args.miningKey.should.be.equal(miningKey);
     })
+    it('should not let create metadata if fullAddress is too long', async () => {
+      let localFakeData = fakeData.slice();
+      localFakeData[3] = 'a'.repeat(201);
+      await metadata.createMetadata(...localFakeData, {from: votingKey}).should.be.rejectedWith(ERROR_MSG);
+      localFakeData[3] = 'a'.repeat(200);
+      await metadata.createMetadata(...localFakeData, {from: votingKey}).should.be.fulfilled;
+    });
     it('should not let create metadata if called by non-voting key', async () => {
       const {logs} = await metadata.createMetadata(...fakeData, {from: miningKey}).should.be.rejectedWith(ERROR_MSG);
       const validators = await metadata.validators.call(miningKey);

test/metadata_upgrade_test.js

@@ -114,6 +114,13 @@ contract('ValidatorMetadata upgraded [all features]', function (accounts) {
       logs[0].event.should.be.equal('MetadataCreated');
       logs[0].args.miningKey.should.be.equal(miningKey);
     })
+    it('should not let create metadata if fullAddress is too long', async () => {
+      let localFakeData = fakeData.slice();
+      localFakeData[3] = 'a'.repeat(201);
+      await metadata.createMetadata(...localFakeData, {from: votingKey}).should.be.rejectedWith(ERROR_MSG);
+      localFakeData[3] = 'a'.repeat(200);
+      await metadata.createMetadata(...localFakeData, {from: votingKey}).should.be.fulfilled;
+    });
     it('should not let create metadata if called by non-voting key', async () => {
       const {logs} = await metadata.createMetadata(...fakeData, {from: miningKey}).should.be.rejectedWith(ERROR_MSG);
       const validators = await metadata.validators.call(miningKey);