diff --git a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol index d8cd017121..6c9eaa7505 100644 --- a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol +++ b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol @@ -118,6 +118,18 @@ library LibStakingRichErrors { bytes4 internal constant POOL_ALREADY_EXISTS_ERROR_SELECTOR = 0x2a5e4dcf; + // bytes4(keccak256("EthVaultNotSet()")) + bytes4 internal constant ETH_VAULT_NOT_SET = + 0xdb3f0be8; + + // bytes4(keccak256("RewardVaultNotSet()")) + bytes4 internal constant REWARD_VAULT_NOT_SET = + 0xfcb260f7; + + // bytes4(keccak256("InvalidStakeState(uint256)")) + bytes4 internal constant INVALID_STAKE_STATE = + 0xe6586728; + // solhint-disable func-name-mixedcase function MiscalculatedRewardsError( uint256 totalRewardsPaid, @@ -443,4 +455,35 @@ library LibStakingRichErrors { poolId ); } + + function EthVaultNotSet() + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + ETH_VAULT_NOT_SET + ); + } + + function RewardVaultNotSet() + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + REWARD_VAULT_NOT_SET + ); + } + + function InvalidStakeState(uint256 state) + internal + pure + returns (bytes memory) + { + return abi.encodeWithSelector( + INVALID_STAKE_STATE, + state + ); + } } diff --git a/contracts/staking/contracts/src/stake/MixinStake.sol b/contracts/staking/contracts/src/stake/MixinStake.sol index a9d719dad6..c05b22dbb6 100644 --- a/contracts/staking/contracts/src/stake/MixinStake.sol +++ b/contracts/staking/contracts/src/stake/MixinStake.sol @@ -27,6 +27,7 @@ import "./MixinZrxVault.sol"; import "../staking_pools/MixinStakingPoolRewardVault.sol"; import "../staking_pools/MixinStakingPoolRewards.sol"; import "../sys/MixinScheduler.sol"; +import "../libs/LibStakingRichErrors.sol"; import "./MixinStakeBalances.sol"; import "./MixinStakeStorage.sol"; @@ -78,10 +79,14 @@ contract MixinStake is // sanity check uint256 currentWithdrawableStake = getWithdrawableStake(owner); - require( - amount <= currentWithdrawableStake, - "INSUFFICIENT_FUNDS" - ); + if (amount > currentWithdrawableStake) { + LibRichErrors.rrevert( + LibStakingRichErrors.InsufficientBalanceError( + amount, + currentWithdrawableStake + ) + ); + } // burn inactive stake _burnBalance(inactiveStakeByOwner[owner], amount); @@ -219,6 +224,7 @@ contract MixinStake is /// @return a storage pointer to the corresponding stake stake function _getBalancePtrFromState(IStructs.StakeState state) private + view returns (IStructs.DelayedBalance storage) { // lookup state @@ -231,7 +237,12 @@ contract MixinStake is return delegatedStakeByOwner[owner]; } - // not found - revert("Unrecognized State"); + // invalid state + LibRichErrors.rrevert( + LibStakingRichErrors.InvalidStakeState(uint256(state)) + ); + + // required to compile ~ we should never hit this. + revert("INVALID_STATE"); } } diff --git a/contracts/staking/contracts/src/stake/MixinStakeStorage.sol b/contracts/staking/contracts/src/stake/MixinStakeStorage.sol index 8ec7dbebdd..a0e13dd4cd 100644 --- a/contracts/staking/contracts/src/stake/MixinStakeStorage.sol +++ b/contracts/staking/contracts/src/stake/MixinStakeStorage.sol @@ -63,7 +63,12 @@ contract MixinStakeStorage is // sanity check on balance if (amount > from.next) { - revert("Insufficient Balance"); + LibRichErrors.rrevert( + LibStakingRichErrors.InsufficientBalanceError( + amount, + from.next + ) + ); } // move stake for next epoch @@ -183,6 +188,7 @@ contract MixinStakeStorage is IStructs.DelayedBalance storage balancePtrB ) private + pure returns (bool areEqual) { assembly { diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewardVault.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewardVault.sol index 6697b8bf33..580d6ef669 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewardVault.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewardVault.sol @@ -21,6 +21,7 @@ pragma solidity ^0.5.9; import "../interfaces/IStakingEvents.sol"; import "../interfaces/IStakingPoolRewardVault.sol"; import "../immutable/MixinStorage.sol"; +import "../libs/LibStakingRichErrors.sol"; /// @dev This mixin contains logic for interfacing with the Staking Pool Reward Vault (vaults/StakingPoolRewardVault.sol) @@ -71,7 +72,15 @@ contract MixinStakingPoolRewardVault is function _depositIntoStakingPoolRewardVault(uint256 amount) internal { + // cast to payable and sanity check address payable rewardVaultAddress = address(uint160(address(rewardVault))); + if (rewardVaultAddress == NIL_ADDRESS) { + LibRichErrors.rrevert( + LibStakingRichErrors.RewardVaultNotSet() + ); + } + + // perform transfer rewardVaultAddress.transfer(amount); } @@ -86,11 +95,15 @@ contract MixinStakingPoolRewardVault is ) internal { + // sanity check IStakingPoolRewardVault _rewardVault = rewardVault; - require( - address(_rewardVault) != NIL_ADDRESS, - "REWARD_VAULT_NOT_SET" - ); + if (address(_rewardVault) == NIL_ADDRESS) { + LibRichErrors.rrevert( + LibStakingRichErrors.RewardVaultNotSet() + ); + } + + // perform transfer _rewardVault.transferMemberBalanceToEthVault(poolId, member, amount); } } diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol index fa2dd55b75..fe62fa598a 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPoolRewards.sol @@ -232,6 +232,7 @@ contract MixinStakingPoolRewards is /// @dev returns true iff Cumulative Rewards are set function _isCumulativeRewardSet(IStructs.Fraction memory cumulativeReward) private + pure returns (bool) { // we use the denominator as a proxy for whether the cumulative diff --git a/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol b/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol index e37aae9c0a..2b9ad53a94 100644 --- a/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol +++ b/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol @@ -120,13 +120,6 @@ contract StakingPoolRewardVault is return; } - // sanity check on eth vault - IEthVault _ethVault = ethVault; - require( - address(_ethVault) != address(0), - "ETH_VAULT_NOT_SET" - ); - // sanity check - sufficient balance? uint256 operatorBalance = uint256(balanceByPoolId[poolId].operatorBalance); if (amount > operatorBalance) { @@ -138,7 +131,7 @@ contract StakingPoolRewardVault is // update balance and transfer `amount` in ETH to staking contract balanceByPoolId[poolId].operatorBalance = operatorBalance.safeSub(amount).downcastToUint96(); - _ethVault.depositFor.value(amount)(operator); + _transferToEthVault(operator, amount); // notify emit RewardWithdrawnForOperator(poolId, amount); @@ -157,12 +150,9 @@ contract StakingPoolRewardVault is external onlyStakingContract { - // sanity check on eth vault - IEthVault _ethVault = ethVault; - require( - address(_ethVault) != address(0), - "ETH_VAULT_NOT_SET" - ); + if (amount == 0) { + return; + } // sanity check - sufficient balance? uint256 membersBalance = uint256(balanceByPoolId[poolId].membersBalance); @@ -175,7 +165,7 @@ contract StakingPoolRewardVault is // update balance and transfer `amount` in ETH to staking contract balanceByPoolId[poolId].membersBalance = membersBalance.safeSub(amount).downcastToUint96(); - _ethVault.depositFor.value(amount)(member); + _transferToEthVault(member, amount); // notify emit RewardWithdrawnForMember(poolId, amount); @@ -296,4 +286,19 @@ contract StakingPoolRewardVault is poolPortion ); } + + function _transferToEthVault(address from, uint256 amount) + private + { + // sanity check on eth vault + IEthVault _ethVault = ethVault; + if (address(_ethVault) == address(0)) { + LibRichErrors.rrevert( + LibStakingRichErrors.EthVaultNotSet() + ); + } + + // perform xfer + _ethVault.depositFor.value(amount)(from); + } } diff --git a/contracts/staking/test/stake_test.ts b/contracts/staking/test/stake_test.ts index 96ea553f6a..62f5cf933d 100644 --- a/contracts/staking/test/stake_test.ts +++ b/contracts/staking/test/stake_test.ts @@ -1,6 +1,7 @@ import { ERC20ProxyContract, ERC20Wrapper } from '@0x/contracts-asset-proxy'; import { DummyERC20TokenContract } from '@0x/contracts-erc20'; import { blockchainTests, describe, provider, web3Wrapper } from '@0x/contracts-test-utils'; +import { StakingRevertErrors } from '@0x/order-utils'; import { BigNumber, StringRevertError } from '@0x/utils'; import * as _ from 'lodash'; @@ -94,7 +95,7 @@ blockchainTests.resets.only('Stake States', () => { { state: StakeState.Active }, { state: StakeState.Inactive }, amount, - new StringRevertError('Insufficient Balance'), + new StakingRevertErrors.InsufficientBalanceError(amount, ZERO), ); }); it('should fail to reassign stake', async () => { @@ -113,7 +114,7 @@ blockchainTests.resets.only('Stake States', () => { { state: StakeState.Inactive }, { state: StakeState.Active }, amount, - new StringRevertError('Insufficient Balance'), + new StakingRevertErrors.InsufficientBalanceError(amount, ZERO), ); }); }); @@ -254,20 +255,20 @@ blockchainTests.resets.only('Stake States', () => { it('should fail to unstake with insufficient balance', async () => { const amount = StakingWrapper.toBaseUnitAmount(10); await staker.stakeAsync(amount); - await staker.unstakeAsync(amount, new StringRevertError('INSUFFICIENT_FUNDS')); + await staker.unstakeAsync(amount, new StakingRevertErrors.InsufficientBalanceError(amount, ZERO)); }); it('should fail to unstake in the same epoch as stake was set to inactive', async () => { const amount = StakingWrapper.toBaseUnitAmount(10); await staker.stakeAsync(amount); await staker.moveStakeAsync({ state: StakeState.Active }, { state: StakeState.Inactive }, amount); - await staker.unstakeAsync(amount, new StringRevertError('INSUFFICIENT_FUNDS')); + await staker.unstakeAsync(amount, new StakingRevertErrors.InsufficientBalanceError(amount, ZERO)); }); it('should fail to unstake after being inactive for <1 epoch', async () => { const amount = StakingWrapper.toBaseUnitAmount(10); await staker.stakeAsync(amount); await staker.moveStakeAsync({ state: StakeState.Active }, { state: StakeState.Inactive }, amount); await staker.goToNextEpochAsync(); - await staker.unstakeAsync(amount, new StringRevertError('INSUFFICIENT_FUNDS')); + await staker.unstakeAsync(amount, new StakingRevertErrors.InsufficientBalanceError(amount, ZERO)); }); it('should fail to unstake in same epoch that inactive/withdrawable stake has been reactivated', async () => { const amount = StakingWrapper.toBaseUnitAmount(10); @@ -276,7 +277,7 @@ blockchainTests.resets.only('Stake States', () => { await staker.goToNextEpochAsync(); // stake is now inactive await staker.goToNextEpochAsync(); // stake is now withdrawable await staker.moveStakeAsync({ state: StakeState.Inactive }, { state: StakeState.Active }, amount); - await staker.unstakeAsync(amount, new StringRevertError('INSUFFICIENT_FUNDS')); + await staker.unstakeAsync(amount, new StakingRevertErrors.InsufficientBalanceError(amount, ZERO)); }); it('should fail to unstake one epoch after inactive/withdrawable stake has been reactivated', async () => { const amount = StakingWrapper.toBaseUnitAmount(10); @@ -286,7 +287,7 @@ blockchainTests.resets.only('Stake States', () => { await staker.goToNextEpochAsync(); // stake is now withdrawable await staker.moveStakeAsync({ state: StakeState.Inactive }, { state: StakeState.Active }, amount); await staker.goToNextEpochAsync(); // stake is active and not withdrawable - await staker.unstakeAsync(amount, new StringRevertError('INSUFFICIENT_FUNDS')); + await staker.unstakeAsync(amount, new StakingRevertErrors.InsufficientBalanceError(amount, ZERO)); }); it('should fail to unstake >1 epoch after inactive/withdrawable stake has been reactivated', async () => { const amount = StakingWrapper.toBaseUnitAmount(10); @@ -297,7 +298,7 @@ blockchainTests.resets.only('Stake States', () => { await staker.moveStakeAsync({ state: StakeState.Inactive }, { state: StakeState.Active }, amount); await staker.goToNextEpochAsync(); // stake is active and not withdrawable await staker.goToNextEpochAsync(); // stake is active and not withdrawable - await staker.unstakeAsync(amount, new StringRevertError('INSUFFICIENT_FUNDS')); + await staker.unstakeAsync(amount, new StakingRevertErrors.InsufficientBalanceError(amount, ZERO)); }); }); describe('Simulations', () => { diff --git a/packages/order-utils/src/staking_revert_errors.ts b/packages/order-utils/src/staking_revert_errors.ts index 9a8ac68e51..7888c5d0d4 100644 --- a/packages/order-utils/src/staking_revert_errors.ts +++ b/packages/order-utils/src/staking_revert_errors.ts @@ -195,6 +195,24 @@ export class PoolAlreadyExistsError extends RevertError { } } +export class EthVaultNotSet extends RevertError { + constructor() { + super('EthVaultNotSet', 'EthVaultNotSet()'); + } +} + +export class RewardVaultNotSet extends RevertError { + constructor() { + super('RewardVaultNotSet', 'RewardVaultNotSet()'); + } +} + +export class InvalidStakeState extends RevertError { + constructor(state?: BigNumber) { + super('InvalidStakeState', 'InvalidStakeState(uint256 state)', { state }); + } +} + const types = [ MiscalculatedRewardsError, OnlyCallableByExchangeError, @@ -220,6 +238,9 @@ const types = [ AmountExceedsBalanceOfPoolError, OperatorShareMustBeBetween0And100Error, PoolAlreadyExistsError, + EthVaultNotSet, + RewardVaultNotSet, + InvalidStakeState ]; // Register the types we've defined.