diff --git a/contracts/staking/contracts/src/interfaces/IStakingPoolRewardVault.sol b/contracts/staking/contracts/src/interfaces/IStakingPoolRewardVault.sol index 3e14983c48..1e98e79580 100644 --- a/contracts/staking/contracts/src/interfaces/IStakingPoolRewardVault.sol +++ b/contracts/staking/contracts/src/interfaces/IStakingPoolRewardVault.sol @@ -28,12 +28,10 @@ pragma solidity ^0.5.9; interface IStakingPoolRewardVault { /// @dev Holds the balances and other data for a staking pool. - /// @param initialzed True iff the balance struct is initialized. /// @param operatorShare Fraction of the total balance owned by the operator, in ppm. /// @param operatorBalance Balance in ETH of the operator. /// @param membersBalance Balance in ETH co-owned by the pool members. struct Pool { - bool initialized; uint32 operatorShare; uint96 operatorBalance; uint96 membersBalance; diff --git a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol index 3daf14c8cb..5a5aa05088 100644 --- a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol +++ b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol @@ -117,9 +117,9 @@ library LibStakingRichErrors { bytes4 internal constant OPERATOR_SHARE_ERROR_SELECTOR = 0x22df9597; - // bytes4(keccak256("PoolAlreadyExistsError(bytes32)")) - bytes4 internal constant POOL_ALREADY_EXISTS_ERROR_SELECTOR = - 0x2a5e4dcf; + // bytes4(keccak256("PoolExistenceError(bytes32,bool)")) + bytes4 internal constant POOL_EXISTENCE_ERROR_SELECTOR = + 0x9ae94f01; // bytes4(keccak256("EthVaultNotSetError()")) bytes4 internal constant ETH_VAULT_NOT_SET_ERROR_SELECTOR = @@ -367,16 +367,18 @@ library LibStakingRichErrors { ); } - function PoolAlreadyExistsError( - bytes32 poolId + function PoolExistenceError( + bytes32 poolId, + bool alreadyExists ) internal pure returns (bytes memory) { return abi.encodeWithSelector( - POOL_ALREADY_EXISTS_ERROR_SELECTOR, - poolId + POOL_EXISTENCE_ERROR_SELECTOR, + poolId, + alreadyExists ); } diff --git a/contracts/staking/contracts/src/stake/MixinStake.sol b/contracts/staking/contracts/src/stake/MixinStake.sol index 0603d780a2..6d68d95e0b 100644 --- a/contracts/staking/contracts/src/stake/MixinStake.sol +++ b/contracts/staking/contracts/src/stake/MixinStake.sol @@ -181,6 +181,16 @@ contract MixinStake is ) private { + // revert if pool with given poolId doesn't exist + if (rewardVault.operatorOf(poolId) == NIL_ADDRESS) { + LibRichErrors.rrevert( + LibStakingRichErrors.PoolExistenceError( + poolId, + false + ) + ); + } + // cache amount delegated to pool by owner IStructs.StoredBalance memory initDelegatedStakeToPoolByOwner = _loadUnsyncedBalance(delegatedStakeToPoolByOwner[owner][poolId]); @@ -196,7 +206,7 @@ contract MixinStake is } /// @dev Un-Delegates a owners stake from a staking pool. - /// @param poolId Id of pool to un-delegate to. + /// @param poolId Id of pool to un-delegate from. /// @param owner who wants to un-delegate. /// @param amount of stake to un-delegate. function _undelegateStake( @@ -206,6 +216,16 @@ contract MixinStake is ) private { + // revert if pool with given poolId doesn't exist + if (rewardVault.operatorOf(poolId) == NIL_ADDRESS) { + LibRichErrors.rrevert( + LibStakingRichErrors.PoolExistenceError( + poolId, + false + ) + ); + } + // cache amount delegated to pool by owner IStructs.StoredBalance memory initDelegatedStakeToPoolByOwner = _loadUnsyncedBalance(delegatedStakeToPoolByOwner[owner][poolId]); diff --git a/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol b/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol index 63d13c4187..b3646bcd59 100644 --- a/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol +++ b/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol @@ -193,16 +193,16 @@ contract StakingPoolRewardVault is )); } - // pool must not exist + // pool must not already exist Pool storage pool = poolById[poolId]; - if (pool.initialized) { - LibRichErrors.rrevert(LibStakingRichErrors.PoolAlreadyExistsError( - poolId + if (pool.operatorAddress != NIL_ADDRESS) { + LibRichErrors.rrevert(LibStakingRichErrors.PoolExistenceError( + poolId, + true )); } // initialize pool - pool.initialized = true; pool.operatorAddress = operatorAddress; pool.operatorShare = operatorShare; diff --git a/contracts/staking/test/stake_test.ts b/contracts/staking/test/stake_test.ts index e30f84f624..df8c41b19b 100644 --- a/contracts/staking/test/stake_test.ts +++ b/contracts/staking/test/stake_test.ts @@ -25,6 +25,7 @@ blockchainTests.resets('Stake Statuses', env => { // stake actor let staker: StakerActor; let poolIds: string[]; + let unusedPoolId: string; let poolOperator: string; // tests before(async () => { @@ -45,6 +46,7 @@ blockchainTests.resets('Stake Statuses', env => { await stakingApiWrapper.utils.createStakingPoolAsync(poolOperator, 4, false), await stakingApiWrapper.utils.createStakingPoolAsync(poolOperator, 5, false), ]); + unusedPoolId = await stakingApiWrapper.stakingContract.getNextStakingPoolId.callAsync(); }); describe('Stake', () => { it('should successfully stake zero ZRX', async () => { @@ -288,6 +290,52 @@ blockchainTests.resets('Stake Statuses', env => { new StakeInfo(StakeStatus.Delegated, poolIds[1]), ); }); + it('active -> delegated (non-existent pool)', async () => { + const amount = toBaseUnitAmount(10); + await staker.stakeAsync(amount); + await staker.moveStakeAsync( + new StakeInfo(StakeStatus.Active), + new StakeInfo(StakeStatus.Delegated, unusedPoolId), + amount, + new StakingRevertErrors.PoolExistenceError(unusedPoolId, false), + ); + }); + it('inactive -> delegated (non-existent pool)', async () => { + const amount = toBaseUnitAmount(10); + await staker.stakeAsync(amount); + await staker.moveStakeAsync(new StakeInfo(StakeStatus.Active), new StakeInfo(StakeStatus.Inactive), amount); + await staker.moveStakeAsync( + new StakeInfo(StakeStatus.Inactive), + new StakeInfo(StakeStatus.Delegated, unusedPoolId), + amount, + new StakingRevertErrors.PoolExistenceError(unusedPoolId, false), + ); + }); + it('delegated -> delegated (non-existent pool)', async () => { + const amount = toBaseUnitAmount(10); + await staker.stakeAsync(amount); + await staker.moveStakeAsync( + new StakeInfo(StakeStatus.Active), + new StakeInfo(StakeStatus.Delegated, poolIds[0]), + amount, + ); + await staker.moveStakeAsync( + new StakeInfo(StakeStatus.Delegated, poolIds[0]), + new StakeInfo(StakeStatus.Delegated, unusedPoolId), + amount, + new StakingRevertErrors.PoolExistenceError(unusedPoolId, false), + ); + }); + it('delegated (non-existent pool) -> active', async () => { + const amount = toBaseUnitAmount(10); + await staker.stakeAsync(amount); + await staker.moveStakeAsync( + new StakeInfo(StakeStatus.Delegated, unusedPoolId), + new StakeInfo(StakeStatus.Active), + amount, + new StakingRevertErrors.PoolExistenceError(unusedPoolId, false), + ); + }); }); describe('Unstake', () => { it('should successfully unstake zero ZRX', async () => { diff --git a/contracts/staking/test/vaults_test.ts b/contracts/staking/test/vaults_test.ts index 78cb81ae5e..6efd17bad8 100644 --- a/contracts/staking/test/vaults_test.ts +++ b/contracts/staking/test/vaults_test.ts @@ -34,7 +34,7 @@ blockchainTests('Staking Vaults', env => { const poolId = await stakingApiWrapper.utils.createStakingPoolAsync(poolOperator, operatorShare, true); const notStakingContractAddress = poolOperator; // should fail to create pool if it already exists - let revertError = new StakingRevertErrors.PoolAlreadyExistsError(poolId); + let revertError = new StakingRevertErrors.PoolExistenceError(poolId, true); let tx = stakingApiWrapper.rewardVaultContract.registerStakingPool.awaitTransactionSuccessAsync( poolId, poolOperator, diff --git a/packages/order-utils/src/staking_revert_errors.ts b/packages/order-utils/src/staking_revert_errors.ts index 9955d5675e..c3a6e849a4 100644 --- a/packages/order-utils/src/staking_revert_errors.ts +++ b/packages/order-utils/src/staking_revert_errors.ts @@ -173,9 +173,12 @@ export class OperatorShareError extends RevertError { } } -export class PoolAlreadyExistsError extends RevertError { - constructor(poolId?: string) { - super('PoolAlreadyExistsError', 'PoolAlreadyExistsError(bytes32 poolId)', { poolId }); +export class PoolExistenceError extends RevertError { + constructor(poolId?: string, alreadyExists?: boolean) { + super('PoolExistenceError', 'PoolExistenceError(bytes32 poolId, bool alreadyExists)', { + poolId, + alreadyExists, + }); } } @@ -266,7 +269,7 @@ const types = [ OnlyCallableIfInCatastrophicFailureError, OnlyCallableIfNotInCatastrophicFailureError, OperatorShareError, - PoolAlreadyExistsError, + PoolExistenceError, RewardVaultNotSetError, WithdrawAmountExceedsMemberBalanceError, ProxyDestinationCannotBeNilError,