From 9ac715f99d445346e22b4990e586a436a6c6cac6 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Tue, 22 Oct 2019 11:25:19 -0700 Subject: [PATCH 1/2] Protocol fee amount is not enforced by staking contract --- .../staking/contracts/src/fees/MixinExchangeFees.sol | 11 ++--------- .../contracts/src/libs/LibStakingRichErrors.sol | 1 - 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/contracts/staking/contracts/src/fees/MixinExchangeFees.sol b/contracts/staking/contracts/src/fees/MixinExchangeFees.sol index 576fbe22db..a5af8526c5 100644 --- a/contracts/staking/contracts/src/fees/MixinExchangeFees.sol +++ b/contracts/staking/contracts/src/fees/MixinExchangeFees.sol @@ -161,15 +161,8 @@ contract MixinExchangeFees is private view { - if (protocolFeePaid == 0) { - LibRichErrors.rrevert( - LibStakingRichErrors.InvalidProtocolFeePaymentError( - LibStakingRichErrors.ProtocolFeePaymentErrorCodes.ZeroProtocolFeePaid, - protocolFeePaid, - msg.value - ) - ); - } + // The protocol fee must equal the value passed to the contract; unless + // the value is zero, in which case the fee is taken in WETH. if (msg.value != protocolFeePaid && msg.value != 0) { LibRichErrors.rrevert( LibStakingRichErrors.InvalidProtocolFeePaymentError( diff --git a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol index 30117d9ce8..83ab9c52b2 100644 --- a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol +++ b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol @@ -30,7 +30,6 @@ library LibStakingRichErrors { } enum ProtocolFeePaymentErrorCodes { - ZeroProtocolFeePaid, MismatchedFeeAndPayment } From c44e16a88f857e7e62858bce04dbc5eff4be9f25 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Tue, 22 Oct 2019 11:53:00 -0700 Subject: [PATCH 2/2] Removed assertion that protocol fee != 0 from staking contract. --- contracts/staking/CHANGELOG.json | 8 ++++++++ .../contracts/src/fees/MixinExchangeFees.sol | 1 - .../src/libs/LibStakingRichErrors.sol | 10 ++-------- .../test/unit_tests/protocol_fees_test.ts | 18 ------------------ packages/order-utils/CHANGELOG.json | 4 ++++ .../order-utils/src/staking_revert_errors.ts | 10 ++-------- 6 files changed, 16 insertions(+), 35 deletions(-) diff --git a/contracts/staking/CHANGELOG.json b/contracts/staking/CHANGELOG.json index 51328f4e8c..4cdc721b0d 100644 --- a/contracts/staking/CHANGELOG.json +++ b/contracts/staking/CHANGELOG.json @@ -5,6 +5,14 @@ { "note": "Add more overflow safeguards to `LibFixedMath`", "pr": 2255 + }, + { + "note": "Refactored finalization state.", + "pr": 2276 + }, + { + "note": "Removed protocol fee != 0 assertion.", + "pr": 2278 } ] }, diff --git a/contracts/staking/contracts/src/fees/MixinExchangeFees.sol b/contracts/staking/contracts/src/fees/MixinExchangeFees.sol index a5af8526c5..e4f33bb5f1 100644 --- a/contracts/staking/contracts/src/fees/MixinExchangeFees.sol +++ b/contracts/staking/contracts/src/fees/MixinExchangeFees.sol @@ -166,7 +166,6 @@ contract MixinExchangeFees is if (msg.value != protocolFeePaid && msg.value != 0) { LibRichErrors.rrevert( LibStakingRichErrors.InvalidProtocolFeePaymentError( - LibStakingRichErrors.ProtocolFeePaymentErrorCodes.MismatchedFeeAndPayment, protocolFeePaid, msg.value ) diff --git a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol index 83ab9c52b2..6acca6d6b7 100644 --- a/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol +++ b/contracts/staking/contracts/src/libs/LibStakingRichErrors.sol @@ -29,10 +29,6 @@ library LibStakingRichErrors { CanOnlyDecreaseOperatorShare } - enum ProtocolFeePaymentErrorCodes { - MismatchedFeeAndPayment - } - enum InitializationErrorCodes { MixinSchedulerAlreadyInitialized, MixinParamsAlreadyInitialized @@ -103,9 +99,9 @@ library LibStakingRichErrors { bytes4 internal constant INVALID_PARAM_VALUE_ERROR_SELECTOR = 0xfc45bd11; - // bytes4(keccak256("InvalidProtocolFeePaymentError(uint8,uint256,uint256)")) + // bytes4(keccak256("InvalidProtocolFeePaymentError(uint256,uint256)")) bytes4 internal constant INVALID_PROTOCOL_FEE_PAYMENT_ERROR_SELECTOR = - 0xefd6cb33; + 0x31d7a505; // bytes4(keccak256("PreviousEpochNotFinalizedError(uint256,uint256)")) bytes4 internal constant PREVIOUS_EPOCH_NOT_FINALIZED_ERROR_SELECTOR = @@ -251,7 +247,6 @@ library LibStakingRichErrors { } function InvalidProtocolFeePaymentError( - ProtocolFeePaymentErrorCodes errorCodes, uint256 expectedProtocolFeePaid, uint256 actualProtocolFeePaid ) @@ -261,7 +256,6 @@ library LibStakingRichErrors { { return abi.encodeWithSelector( INVALID_PROTOCOL_FEE_PAYMENT_ERROR_SELECTOR, - errorCodes, expectedProtocolFeePaid, actualProtocolFeePaid ); diff --git a/contracts/staking/test/unit_tests/protocol_fees_test.ts b/contracts/staking/test/unit_tests/protocol_fees_test.ts index d5b5dcd53d..9f3a917910 100644 --- a/contracts/staking/test/unit_tests/protocol_fees_test.ts +++ b/contracts/staking/test/unit_tests/protocol_fees_test.ts @@ -90,21 +90,6 @@ blockchainTests('Protocol Fees unit tests', env => { return expect(tx).to.revertWith(expectedError); }); - it('should revert if `protocolFeePaid` is zero with zero value sent', async () => { - const tx = testContract.payProtocolFee.awaitTransactionSuccessAsync( - makerAddress, - payerAddress, - ZERO_AMOUNT, - { from: exchangeAddress, value: ZERO_AMOUNT }, - ); - const expectedError = new StakingRevertErrors.InvalidProtocolFeePaymentError( - StakingRevertErrors.ProtocolFeePaymentErrorCodes.ZeroProtocolFeePaid, - ZERO_AMOUNT, - ZERO_AMOUNT, - ); - return expect(tx).to.revertWith(expectedError); - }); - it('should revert if `protocolFeePaid` is zero with non-zero value sent', async () => { const tx = testContract.payProtocolFee.awaitTransactionSuccessAsync( makerAddress, @@ -113,7 +98,6 @@ blockchainTests('Protocol Fees unit tests', env => { { from: exchangeAddress, value: DEFAULT_PROTOCOL_FEE_PAID }, ); const expectedError = new StakingRevertErrors.InvalidProtocolFeePaymentError( - StakingRevertErrors.ProtocolFeePaymentErrorCodes.ZeroProtocolFeePaid, ZERO_AMOUNT, DEFAULT_PROTOCOL_FEE_PAID, ); @@ -128,7 +112,6 @@ blockchainTests('Protocol Fees unit tests', env => { { from: exchangeAddress, value: DEFAULT_PROTOCOL_FEE_PAID.minus(1) }, ); const expectedError = new StakingRevertErrors.InvalidProtocolFeePaymentError( - StakingRevertErrors.ProtocolFeePaymentErrorCodes.MismatchedFeeAndPayment, DEFAULT_PROTOCOL_FEE_PAID, DEFAULT_PROTOCOL_FEE_PAID.minus(1), ); @@ -143,7 +126,6 @@ blockchainTests('Protocol Fees unit tests', env => { { from: exchangeAddress, value: DEFAULT_PROTOCOL_FEE_PAID.plus(1) }, ); const expectedError = new StakingRevertErrors.InvalidProtocolFeePaymentError( - StakingRevertErrors.ProtocolFeePaymentErrorCodes.MismatchedFeeAndPayment, DEFAULT_PROTOCOL_FEE_PAID, DEFAULT_PROTOCOL_FEE_PAID.plus(1), ); diff --git a/packages/order-utils/CHANGELOG.json b/packages/order-utils/CHANGELOG.json index aaa6645a01..9cdec9e65e 100644 --- a/packages/order-utils/CHANGELOG.json +++ b/packages/order-utils/CHANGELOG.json @@ -113,6 +113,10 @@ { "note": "Renamed `OnlyCallableByPoolOperatorOrMakerError` to `OnlyCallableByPoolOperatorError`.", "pr": 2250 + }, + { + "note": "Removed protocol fee != 0 error.", + "pr": 2278 } ], "timestamp": 1570135330 diff --git a/packages/order-utils/src/staking_revert_errors.ts b/packages/order-utils/src/staking_revert_errors.ts index 8debcffe8e..81c0c983ee 100644 --- a/packages/order-utils/src/staking_revert_errors.ts +++ b/packages/order-utils/src/staking_revert_errors.ts @@ -14,11 +14,6 @@ export enum OperatorShareErrorCodes { CanOnlyDecreaseOperatorShare, } -export enum ProtocolFeePaymentErrorCodes { - ZeroProtocolFeePaid, - MismatchedFeeAndPayment, -} - export enum InvalidParamValueErrorCodes { InvalidCobbDouglasAlpha, InvalidRewardDelegatedStakeWeight, @@ -144,14 +139,13 @@ export class InvalidParamValueError extends RevertError { export class InvalidProtocolFeePaymentError extends RevertError { constructor( - errorCode?: ProtocolFeePaymentErrorCodes, expectedProtocolFeePaid?: BigNumber | number | string, actualProtocolFeePaid?: BigNumber | number | string, ) { super( 'InvalidProtocolFeePaymentError', - 'InvalidProtocolFeePaymentError(uint8 errorCode, uint256 expectedProtocolFeePaid, uint256 actualProtocolFeePaid)', - { errorCode, expectedProtocolFeePaid, actualProtocolFeePaid }, + 'InvalidProtocolFeePaymentError(uint256 expectedProtocolFeePaid, uint256 actualProtocolFeePaid)', + { expectedProtocolFeePaid, actualProtocolFeePaid }, ); } }