From db241e8f909a3681f63a46824eab2736af1c3ecc Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Mon, 23 Sep 2019 17:32:52 -0700 Subject: [PATCH 1/3] `@0x:contracts-staking` Modified Staking events to improve their usability --- .../contracts/src/interfaces/IStakingEvents.sol | 16 +++------------- .../contracts/src/interfaces/IZrxVault.sol | 4 ---- .../staking/contracts/src/vaults/ZrxVault.sol | 4 ++-- 3 files changed, 5 insertions(+), 19 deletions(-) diff --git a/contracts/staking/contracts/src/interfaces/IStakingEvents.sol b/contracts/staking/contracts/src/interfaces/IStakingEvents.sol index 2e64439bfd..308df12d00 100644 --- a/contracts/staking/contracts/src/interfaces/IStakingEvents.sol +++ b/contracts/staking/contracts/src/interfaces/IStakingEvents.sol @@ -108,16 +108,6 @@ interface IStakingEvents { address zrxVaultAddress ); - /// @dev Emitted by MixinScheduler when the timeLock period is changed. - /// @param timeLockPeriod The timeLock period we changed to. - /// @param startEpoch The epoch this period started. - /// @param endEpoch The epoch this period ends. - event TimeLockPeriodChanged( - uint256 timeLockPeriod, - uint256 startEpoch, - uint256 endEpoch - ); - /// @dev Emitted by MixinStakingPool when a new pool is created. /// @param poolId Unique id generated for pool. /// @param operator The operator (creator) of pool. @@ -132,7 +122,7 @@ interface IStakingEvents { /// @param poolId Unique id of pool. /// @param makerAddress Adress of maker joining the pool. event PendingAddMakerToPool( - bytes32 poolId, + bytes32 indexed poolId, address makerAddress ); @@ -140,7 +130,7 @@ interface IStakingEvents { /// @param poolId Unique id of pool. /// @param makerAddress Adress of maker added to pool. event MakerAddedToStakingPool( - bytes32 poolId, + bytes32 indexed poolId, address makerAddress ); @@ -148,7 +138,7 @@ interface IStakingEvents { /// @param poolId Unique id of pool. /// @param makerAddress Adress of maker added to pool. event MakerRemovedFromStakingPool( - bytes32 poolId, + bytes32 indexed poolId, address makerAddress ); diff --git a/contracts/staking/contracts/src/interfaces/IZrxVault.sol b/contracts/staking/contracts/src/interfaces/IZrxVault.sol index 7d151a5966..190ed4924f 100644 --- a/contracts/staking/contracts/src/interfaces/IZrxVault.sol +++ b/contracts/staking/contracts/src/interfaces/IZrxVault.sol @@ -29,21 +29,17 @@ pragma solidity ^0.5.9; interface IZrxVault { /// @dev Emitted when Zrx Tokens are deposited into the vault. - /// @param sender Address of sender (`msg.sender`). /// @param owner of Zrx Tokens. /// @param amount of Zrx Tokens deposited. event ZrxDepositedIntoVault( - address indexed sender, address indexed owner, uint256 amount ); /// @dev Emitted when Zrx Tokens are withdrawn from the vault. - /// @param sender Address of sender (`msg.sender`). /// @param owner of Zrx Tokens. /// @param amount of Zrx Tokens withdrawn. event ZrxWithdrawnFromVault( - address indexed sender, address indexed owner, uint256 amount ); diff --git a/contracts/staking/contracts/src/vaults/ZrxVault.sol b/contracts/staking/contracts/src/vaults/ZrxVault.sol index 0ff8ef81ab..74e2af494d 100644 --- a/contracts/staking/contracts/src/vaults/ZrxVault.sol +++ b/contracts/staking/contracts/src/vaults/ZrxVault.sol @@ -96,7 +96,7 @@ contract ZrxVault is _balances[owner] = _balances[owner].safeAdd(amount); // notify - emit ZrxDepositedIntoVault(msg.sender, owner, amount); + emit ZrxDepositedIntoVault(owner, amount); // deposit ZRX from owner zrxAssetProxy.transferFrom( @@ -158,7 +158,7 @@ contract ZrxVault is _balances[owner] = _balances[owner].safeSub(amount); // notify - emit ZrxWithdrawnFromVault(msg.sender, owner, amount); + emit ZrxWithdrawnFromVault(owner, amount); // withdraw ZRX to owner _zrxToken.transfer( From 970f77beb0b4b5938612d5eb7993f9afe796045b Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Mon, 23 Sep 2019 17:33:29 -0700 Subject: [PATCH 2/3] `@0x:contracts-staking` Added storage layout assertions to the staking contract --- contracts/staking/contracts/src/Staking.sol | 337 ++++++++++++++++++++ 1 file changed, 337 insertions(+) diff --git a/contracts/staking/contracts/src/Staking.sol b/contracts/staking/contracts/src/Staking.sol index 6df9c8806e..256b670fff 100644 --- a/contracts/staking/contracts/src/Staking.sol +++ b/contracts/staking/contracts/src/Staking.sol @@ -75,4 +75,341 @@ contract Staking is _zrxVaultAddress ); } + + /// @dev This function will fail if the storage layout of this contract deviates from + /// the original staking contract's storage. The use of this function provides assurance + /// that regressions from the original storage layout will not occur. + function _assertStorageLayout() + internal + pure + { + assembly { + let slot := 0x0 + let offset := 0x0 + + /// Ownable + + assertSlotAndOffset( + owner_slot, + owner_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + /// Authorizable + + assertSlotAndOffset( + authorized_slot, + authorized_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + authorities_slot, + authorities_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + /// MixinStorage + + assertSlotAndOffset( + wethAssetProxy_slot, + wethAssetProxy_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + stakingContract_slot, + stakingContract_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + readOnlyProxy_slot, + readOnlyProxy_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + readOnlyProxyCallee_slot, + readOnlyProxyCallee_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + globalStakeByStatus_slot, + globalStakeByStatus_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + _activeStakeByOwner_slot, + _activeStakeByOwner_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + _inactiveStakeByOwner_slot, + _inactiveStakeByOwner_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + _delegatedStakeByOwner_slot, + _delegatedStakeByOwner_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + _delegatedStakeToPoolByOwner_slot, + _delegatedStakeToPoolByOwner_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + _delegatedStakeByPoolId_slot, + _delegatedStakeByPoolId_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + _withdrawableStakeByOwner_slot, + _withdrawableStakeByOwner_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + nextPoolId_slot, + nextPoolId_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + poolJoinedByMakerAddress_slot, + poolJoinedByMakerAddress_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + _poolById_slot, + _poolById_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + rewardsByPoolId_slot, + rewardsByPoolId_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + currentEpoch_slot, + currentEpoch_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + currentEpochStartTimeInSeconds_slot, + currentEpochStartTimeInSeconds_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + _cumulativeRewardsByPool_slot, + _cumulativeRewardsByPool_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + _cumulativeRewardsByPoolReferenceCounter_slot, + _cumulativeRewardsByPoolReferenceCounter_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + _cumulativeRewardsByPoolLastStored_slot, + _cumulativeRewardsByPoolLastStored_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + validExchanges_slot, + validExchanges_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + zrxVault_slot, + zrxVault_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + epochDurationInSeconds_slot, + epochDurationInSeconds_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + rewardDelegatedStakeWeight_slot, + rewardDelegatedStakeWeight_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + minimumPoolStake_slot, + minimumPoolStake_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + maximumMakersInPool_slot, + maximumMakersInPool_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + cobbDouglasAlphaNumerator_slot, + cobbDouglasAlphaNumerator_offset, + slot, + offset + ) + offset := add(offset, 0x4) + + // This number will be tightly packed into the previous values storage slot since + // they are both `uint32`. Because of this tight packing, the offset of this value + // must be 4, since the previous value is a 4 byte number. + assertSlotAndOffset( + cobbDouglasAlphaDenominator_slot, + cobbDouglasAlphaDenominator_offset, + slot, + offset + ) + slot := add(slot, 0x1) + offset := 0x0 + + assertSlotAndOffset( + totalFeesCollectedThisEpoch_slot, + totalFeesCollectedThisEpoch_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + totalWeightedStakeThisEpoch_slot, + totalWeightedStakeThisEpoch_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + _activePoolsByEpoch_slot, + _activePoolsByEpoch_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + numActivePoolsThisEpoch_slot, + numActivePoolsThisEpoch_offset, + slot, + offset + ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + unfinalizedState_slot, + unfinalizedState_offset, + slot, + offset + ) + + // This assembly function will assert that the actual values for `_slot` and `_offset` are + // correct and will revert with a rich error if they are different than the expected values. + function assertSlotAndOffset( + actual_slot, + actual_offset, + expected_slot, + expected_offset + ) { + // If expected_slot is not equal to actual_slot, revert with a rich error. + if iszero(eq(expected_slot, actual_slot)) { + mstore(0x0, 0x213eb13400000000000000000000000000000000000000000000000000000000) // Rich error selector + mstore(0x4, 0x0) // Unexpected slot error code + mstore(0x24, expected_slot) // Expected slot + mstore(0x44, actual_slot) // Actual slot + revert(0x0, 0x64) + } + + // If expected_offset is not equal to actual_offset, revert with a rich error. + if iszero(eq(expected_offset, actual_offset)) { + mstore(0x0, 0x213eb13400000000000000000000000000000000000000000000000000000000) // Rich error selector + mstore(0x4, 0x1) // Unexpected offset error code + mstore(0x24, expected_offset) // Expected offset + mstore(0x44, actual_offset) // Actual offset + revert(0x0, 0x64) + } + } + } + } } From ef04248191d3050b183ce511d4b92492524c9c3f Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Tue, 24 Sep 2019 10:58:51 -0700 Subject: [PATCH 3/3] `@0x:contracts-staking` Addressed review comments --- contracts/staking/contracts/src/Staking.sol | 8 ++++++++ contracts/staking/contracts/src/interfaces/IZrxVault.sol | 4 ++-- contracts/staking/contracts/src/vaults/ZrxVault.sol | 4 ++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/contracts/staking/contracts/src/Staking.sol b/contracts/staking/contracts/src/Staking.sol index 256b670fff..a4051400d1 100644 --- a/contracts/staking/contracts/src/Staking.sol +++ b/contracts/staking/contracts/src/Staking.sol @@ -383,6 +383,14 @@ contract Staking is slot, offset ) + slot := add(slot, 0x1) + + assertSlotAndOffset( + _wethReservedForPoolRewards_slot, + _wethReservedForPoolRewards_offset, + slot, + offset + ) // This assembly function will assert that the actual values for `_slot` and `_offset` are // correct and will revert with a rich error if they are different than the expected values. diff --git a/contracts/staking/contracts/src/interfaces/IZrxVault.sol b/contracts/staking/contracts/src/interfaces/IZrxVault.sol index 190ed4924f..334db76a4f 100644 --- a/contracts/staking/contracts/src/interfaces/IZrxVault.sol +++ b/contracts/staking/contracts/src/interfaces/IZrxVault.sol @@ -31,7 +31,7 @@ interface IZrxVault { /// @dev Emitted when Zrx Tokens are deposited into the vault. /// @param owner of Zrx Tokens. /// @param amount of Zrx Tokens deposited. - event ZrxDepositedIntoVault( + event Deposit( address indexed owner, uint256 amount ); @@ -39,7 +39,7 @@ interface IZrxVault { /// @dev Emitted when Zrx Tokens are withdrawn from the vault. /// @param owner of Zrx Tokens. /// @param amount of Zrx Tokens withdrawn. - event ZrxWithdrawnFromVault( + event Withdraw( address indexed owner, uint256 amount ); diff --git a/contracts/staking/contracts/src/vaults/ZrxVault.sol b/contracts/staking/contracts/src/vaults/ZrxVault.sol index 74e2af494d..64ca9bcff7 100644 --- a/contracts/staking/contracts/src/vaults/ZrxVault.sol +++ b/contracts/staking/contracts/src/vaults/ZrxVault.sol @@ -96,7 +96,7 @@ contract ZrxVault is _balances[owner] = _balances[owner].safeAdd(amount); // notify - emit ZrxDepositedIntoVault(owner, amount); + emit Deposit(owner, amount); // deposit ZRX from owner zrxAssetProxy.transferFrom( @@ -158,7 +158,7 @@ contract ZrxVault is _balances[owner] = _balances[owner].safeSub(amount); // notify - emit ZrxWithdrawnFromVault(owner, amount); + emit Withdraw(owner, amount); // withdraw ZRX to owner _zrxToken.transfer(