diff --git a/contracts/staking/stakeManager/StakeManager.sol b/contracts/staking/stakeManager/StakeManager.sol index ff8deb117..cb6d19bc9 100644 --- a/contracts/staking/stakeManager/StakeManager.sol +++ b/contracts/staking/stakeManager/StakeManager.sol @@ -203,18 +203,18 @@ contract StakeManager is @dev Change the number of validators required to allow a passed header root */ function updateValidatorThreshold(uint256 newThreshold) public onlyGovernance { - require(newThreshold > 0); + require(newThreshold != 0); logger.logThresholdChange(newThreshold, validatorThreshold); validatorThreshold = newThreshold; } function updateCheckPointBlockInterval(uint256 _blocks) public onlyGovernance { - require(_blocks > 0, "incorrect value"); + require(_blocks != 0, "incorrect value"); checkPointBlockInterval = _blocks; } function updateCheckpointReward(uint256 newReward) public onlyGovernance { - require(newReward > 0); + require(newReward != 0); logger.logRewardUpdate(newReward, CHECKPOINT_REWARD); CHECKPOINT_REWARD = newReward; } @@ -531,7 +531,7 @@ contract StakeManager is } } - function increaseValidatorDelegatedAmount(uint256 validatorId, uint256 amount) public onlyDelegation(validatorId) { + function increaseValidatorDelegatedAmount(uint256 validatorId, uint256 amount) private { validators[validatorId].delegatedAmount = validators[validatorId].delegatedAmount.add(amount); } @@ -540,7 +540,7 @@ contract StakeManager is } function updateSigner(uint256 validatorId, bytes memory signerPubkey) public onlyStaker(validatorId) { - address signer = _pubToAddress(signerPubkey); + address signer = _getAndAssertSigner(signerPubkey); uint256 _currentEpoch = currentEpoch; require(_currentEpoch >= latestSignerUpdateEpoch[validatorId].add(signerUpdateLimit), "Not allowed"); @@ -576,7 +576,7 @@ contract StakeManager is unsignedCtx.totalValidators = signers.length; UnstakedValidatorsContext memory unstakeCtx; - unstakeCtx.deactivatedValidators = new uint256[](totalStakers); + unstakeCtx.deactivatedValidators = new uint256[](signers.length + totalStakers); for (uint256 i = 0; i < sigs.length; ++i) { address signer = ECVerify.ecrecovery(voteHash, sigs[i]); @@ -599,23 +599,26 @@ contract StakeManager is if (_isValidator(status, amount, unstakeCtx.deactivationEpoch, _currentEpoch)) { lastAdd = signer; - signedStakePower = signedStakePower.add(validators[validatorId].delegatedAmount.add(amount)); + signedStakePower = signedStakePower.add(validators[validatorId].delegatedAmount).add(amount); if (unstakeCtx.deactivationEpoch != 0) { + // this validator not a part of signers list anymore unstakeCtx.deactivatedValidators[unstakeCtx.validatorIndex] = validatorId; unstakeCtx.validatorIndex++; } else { - unsignedCtx = _fillUnsignedValidators(unsignedCtx, signer, unsignedCtx.totalValidators); + unsignedCtx = _fillUnsignedValidators(unsignedCtx, signer); } } else if (status == Status.Locked) { + // TODO fix double unsignedValidators appearance // make sure that jailed validator doesn't get his rewards too unsignedCtx.unsignedValidators[unsignedCtx.unsignedValidatorIndex] = validatorId; unsignedCtx.unsignedValidatorIndex++; + unsignedCtx.validatorIndex++; } } // find the rest of validators without signature - unsignedCtx = _fillUnsignedValidators(unsignedCtx, address(0), unsignedCtx.totalValidators); + unsignedCtx = _fillUnsignedValidators(unsignedCtx, address(0)); return _increaseRewardAndAssertConsensus( @@ -771,7 +774,7 @@ contract StakeManager is Private Methods */ - function _pubToAddress(bytes memory pub) private view returns (address) { + function _getAndAssertSigner(bytes memory pub) private view returns (address) { require(pub.length == 64, "not pub"); address signer = address(uint160(uint256(keccak256(pub)))); require(signer != address(0) && signerToValidator[signer] == 0, "Invalid signer"); @@ -787,15 +790,13 @@ contract StakeManager is return (amount > 0 && (deactivationEpoch == 0 || deactivationEpoch > _currentEpoch) && status == Status.Active); } - function _fillUnsignedValidators( - UnsignedValidatorsContext memory context, - address signer, - uint256 totalValidators - ) private view returns (UnsignedValidatorsContext memory) { - while (context.validatorIndex < totalValidators && context.validators[context.validatorIndex] != signer) { - context.unsignedValidators[context.unsignedValidatorIndex] = signerToValidator[ - context.validators[context.validatorIndex] - ]; + function _fillUnsignedValidators(UnsignedValidatorsContext memory context, address signer) + private + view + returns(UnsignedValidatorsContext memory) + { + while (context.validatorIndex < context.totalValidators && context.validators[context.validatorIndex] != signer) { + context.unsignedValidators[context.unsignedValidatorIndex] = signerToValidator[context.validators[context.validatorIndex]]; context.unsignedValidatorIndex++; context.validatorIndex++; } @@ -1052,7 +1053,7 @@ contract StakeManager is bool acceptDelegation, bytes memory signerPubkey ) internal returns (uint256) { - address signer = _pubToAddress(signerPubkey); + address signer = _getAndAssertSigner(signerPubkey); uint256 _currentEpoch = currentEpoch; uint256 validatorId = NFTCounter; StakingInfo _logger = logger; @@ -1115,7 +1116,6 @@ contract StakeManager is _liquidateRewards(validatorId, validator); - // update future uint256 targetEpoch = exitEpoch <= currentEpoch ? 0 : exitEpoch; updateTimeline(-(int256(amount) + delegationAmount), -1, targetEpoch); @@ -1175,7 +1175,8 @@ contract StakeManager is function _insertSigner(address newSigner) internal { signers.push(newSigner); - uint256 i = signers.length - 1; + uint lastIndex = signers.length - 1; + uint i = lastIndex; for (; i > 0; --i) { address signer = signers[i - 1]; if (signer < newSigner) { @@ -1184,12 +1185,14 @@ contract StakeManager is signers[i] = signer; } - signers[i] = newSigner; + if (i != lastIndex) { + signers[i] = newSigner; + } } - function _updateSigner(address prevSigner, address signerToDelete) internal { + function _updateSigner(address prevSigner, address newSigner) internal { _removeSigner(prevSigner); - _insertSigner(signerToDelete); + _insertSigner(newSigner); } function _removeSigner(address signerToDelete) internal { diff --git a/contracts/staking/validatorShare/ValidatorShare.sol b/contracts/staking/validatorShare/ValidatorShare.sol index a3c967128..770a1abd7 100644 --- a/contracts/staking/validatorShare/ValidatorShare.sol +++ b/contracts/staking/validatorShare/ValidatorShare.sol @@ -115,7 +115,7 @@ contract ValidatorShare is IValidatorShare, ERC20NonTradable, OwnableLockable, I require(liquidReward >= minAmount, "Too small rewards to restake"); - if (liquidReward > 0) { + if (liquidReward != 0) { amountRestaked = _buyShares(liquidReward, 0, user); if (liquidReward > amountRestaked) { @@ -257,7 +257,7 @@ contract ValidatorShare is IValidatorShare, ERC20NonTradable, OwnableLockable, I function _sellVoucher(uint256 claimAmount, uint256 maximumSharesToBurn) private returns(uint256, uint256) { // first get how much staked in total and compare to target unstake amount (uint256 totalStaked, uint256 rate) = getTotalStake(msg.sender); - require(totalStaked > 0 && totalStaked >= claimAmount, "Too much requested"); + require(totalStaked != 0 && totalStaked >= claimAmount, "Too much requested"); // convert requested amount back to shares uint256 precision = _getRatePrecision(); @@ -302,7 +302,7 @@ contract ValidatorShare is IValidatorShare, ERC20NonTradable, OwnableLockable, I function _calculateRewardPerShareWithRewards(uint256 accumulatedReward) private view returns (uint256) { uint256 _rewardPerShare = rewardPerShare; - if (accumulatedReward > 0) { + if (accumulatedReward != 0) { _rewardPerShare = _rewardPerShare.add(accumulatedReward.mul(_getRatePrecision()).div(totalSupply())); } @@ -337,7 +337,7 @@ contract ValidatorShare is IValidatorShare, ERC20NonTradable, OwnableLockable, I function _withdrawAndTransferReward(address user) private returns (uint256) { uint256 liquidRewards = _withdrawReward(user); - if (liquidRewards > 0) { + if (liquidRewards != 0) { require(stakeManager.transferFunds(validatorId, liquidRewards, user), "Insufficent rewards"); stakingLogger.logDelegatorClaimRewards(validatorId, user, liquidRewards); } @@ -360,7 +360,7 @@ contract ValidatorShare is IValidatorShare, ERC20NonTradable, OwnableLockable, I _mint(user, shares); // clamp amount of tokens in case resulted shares requires less tokens than anticipated - _amount = _amount.sub(_amount % rate.mul(shares).div(precision)); + _amount = rate.mul(shares).div(precision); stakeManager.updateValidatorState(validatorId, int256(_amount)); diff --git a/test/units/staking/stakeManager/StakeManager.test.js b/test/units/staking/stakeManager/StakeManager.test.js index 639bc9422..6fe320586 100644 --- a/test/units/staking/stakeManager/StakeManager.test.js +++ b/test/units/staking/stakeManager/StakeManager.test.js @@ -451,11 +451,11 @@ contract('StakeManager', async function(accounts) { }) } - describe('when validator unstakes but do not sign last checkpoint', function() { + describe('when 1st validator unstakes but 2nd do not sign a checkpoint', function() { const validatorWallet = wallets[2] const validatorId = '1' const stakers = [ - { wallet: wallets[2], stake: new BN(web3.utils.toWei('200')) }, + { wallet: validatorWallet, stake: new BN(web3.utils.toWei('200')) }, { wallet: wallets[4], stake: new BN(web3.utils.toWei('100')) }, { wallet: wallets[3], stake: new BN(web3.utils.toWei('200')) } ] @@ -478,6 +478,33 @@ contract('StakeManager', async function(accounts) { }) }) + describe('when validator unstakes and do not sign last checkpoint', function() { + const validatorWallet = wallets[4] + const validatorId = '2' + const stakers = [ + { wallet: wallets[2], stake: new BN(web3.utils.toWei('200')) }, + { wallet: validatorWallet, stake: new BN(web3.utils.toWei('100')) }, + { wallet: wallets[3], stake: new BN(web3.utils.toWei('200')) } + ] + + const signers = stakers.map(x => x.wallet) + signers.splice(1, 1) + + prepareToTest(stakers, 1) + + before('must unstake', async function() { + await this.stakeManager.unstake(validatorId, { + from: validatorWallet.getChecksumAddressString() + }) + }) + + testCheckpointing(stakers, signers, 1, 1, { + [stakers[0].wallet.getAddressString()]: '3600000000000000000000', + [stakers[1].wallet.getAddressString()]: '0000000000000000000000', + [stakers[2].wallet.getAddressString()]: '3600000000000000000000' + }) + }) + describe('when validator signs twice and sends his 2nd signature out of order', function() { let stakers = [ { wallet: wallets[2], stake: new BN(web3.utils.toWei('100')) }, @@ -525,11 +552,11 @@ contract('StakeManager', async function(accounts) { }) }) - describe('when 200 validators stake, block interval 1, 2 epochs', function() { + describe('when 100 validators stake, block interval 1, 2 epochs', function() { const stakers = [] - const w = generateFirstWallets(mnemonics, 200) - for (let i = 0; i < 200; ++i) { + const w = generateFirstWallets(mnemonics, 100) + for (let i = 0; i < 100; ++i) { stakers.push({ wallet: w[i], stake: new BN(web3.utils.toWei('1'))