diff --git a/contracts/staking/contracts/src/core/MixinPools.sol b/contracts/staking/contracts/src/core/MixinPools.sol index 939ca303c9..6fbe10ba08 100644 --- a/contracts/staking/contracts/src/core/MixinPools.sol +++ b/contracts/staking/contracts/src/core/MixinPools.sol @@ -23,13 +23,15 @@ import "@0x/contracts-utils/contracts/src/SafeMath.sol"; import "../immutable/MixinConstants.sol"; import "../interfaces/IStakingEvents.sol"; import "./MixinRewardVault.sol"; +import "./MixinSignatureValidator.sol"; contract MixinPools is SafeMath, IStakingEvents, MixinConstants, MixinStorage, - MixinRewardVault + MixinRewardVault, + MixinSignatureValidator { function _getNextPoolId() @@ -65,7 +67,8 @@ contract MixinPools is function _addMakerToPool( bytes32 poolId, address makerAddress, - bytes memory makeSignature, + bytes32 hashSignedByMaker, + bytes memory makerSignature, address operatorAddress ) internal @@ -75,6 +78,11 @@ contract MixinPools is "BAD_POOL_OPERATOR" ); + require( + _isValidSignature(hashSignedByMaker, makerAddress, makerSignature), + "INVALID_MAKER_SIGNATURE" + ); + _recordMaker(poolId, makerAddress); } diff --git a/contracts/staking/contracts/src/core/MixinSignatureValidator.sol b/contracts/staking/contracts/src/core/MixinSignatureValidator.sol new file mode 100644 index 0000000000..15763a299f --- /dev/null +++ b/contracts/staking/contracts/src/core/MixinSignatureValidator.sol @@ -0,0 +1,197 @@ +/* + Copyright 2018 ZeroEx Intl. + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +pragma solidity ^0.5.5; + +import "@0x/contracts-utils/contracts/src/LibBytes.sol"; +import "../interfaces/IStructs.sol"; +import "../interfaces/IWallet.sol"; + + +contract MixinSignatureValidator is + IStructs +{ + using LibBytes for bytes; + + /// @dev Verifies that a hash has been signed by the given signer. + /// @param hash Any 32 byte hash. + /// @param signerAddress Address that should have signed the given hash. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if the address recovered from the provided signature matches the input signer address. + function _isValidSignature( + bytes32 hash, + address signerAddress, + bytes memory signature + ) + internal + view + returns (bool isValid) + { + require( + signature.length > 0, + "LENGTH_GREATER_THAN_0_REQUIRED" + ); + + // Pop last byte off of signature byte array. + uint8 signatureTypeRaw = uint8(signature.popLastByte()); + + // Ensure signature is supported + require( + signatureTypeRaw < uint8(SignatureType.NSignatureTypes), + "SIGNATURE_UNSUPPORTED" + ); + + SignatureType signatureType = SignatureType(signatureTypeRaw); + + // Variables are not scoped in Solidity. + uint8 v; + bytes32 r; + bytes32 s; + address recovered; + + // Always illegal signature. + // This is always an implicit option since a signer can create a + // signature array with invalid type or length. We may as well make + // it an explicit option. This aids testing and analysis. It is + // also the initialization value for the enum type. + if (signatureType == SignatureType.Illegal) { + revert("SIGNATURE_ILLEGAL"); + + // Always invalid signature. + // Like Illegal, this is always implicitly available and therefore + // offered explicitly. It can be implicitly created by providing + // a correctly formatted but incorrect signature. + } else if (signatureType == SignatureType.Invalid) { + require( + signature.length == 0, + "LENGTH_0_REQUIRED" + ); + isValid = false; + return isValid; + + // Signature using EIP712 + } else if (signatureType == SignatureType.EIP712) { + require( + signature.length == 65, + "LENGTH_65_REQUIRED" + ); + v = uint8(signature[0]); + r = signature.readBytes32(1); + s = signature.readBytes32(33); + recovered = ecrecover( + hash, + v, + r, + s + ); + isValid = signerAddress == recovered; + return isValid; + + // Signed using web3.eth_sign + } else if (signatureType == SignatureType.EthSign) { + require( + signature.length == 65, + "LENGTH_65_REQUIRED" + ); + v = uint8(signature[0]); + r = signature.readBytes32(1); + s = signature.readBytes32(33); + recovered = ecrecover( + keccak256(abi.encodePacked( + "\x19Ethereum Signed Message:\n32", + hash + )), + v, + r, + s + ); + isValid = signerAddress == recovered; + return isValid; + + // Signature verified by wallet contract. + // If used with an order, the maker of the order is the wallet contract. + } else if (signatureType == SignatureType.Wallet) { + isValid = isValidWalletSignature( + hash, + signerAddress, + signature + ); + return isValid; + + // Signature verified by wallet contract. + // If used with an order, the maker of the order is the wallet contract. + } else if (signatureType == SignatureType.Wallet) { + isValid = isValidWalletSignature( + hash, + signerAddress, + signature + ); + return isValid; + } + + // Anything else is illegal (We do not return false because + // the signature may actually be valid, just not in a format + // that we currently support. In this case returning false + // may lead the caller to incorrectly believe that the + // signature was invalid.) + revert("SIGNATURE_UNSUPPORTED"); + } + + /// @dev Verifies signature using logic defined by Wallet contract. + /// @param hash Any 32 byte hash. + /// @param walletAddress Address that should have signed the given hash + /// and defines its own signature verification method. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if signature is valid for given wallet.. + function isValidWalletSignature( + bytes32 hash, + address walletAddress, + bytes memory signature + ) + internal + view + returns (bool isValid) + { + bytes memory callData = abi.encodeWithSelector( + IWallet(walletAddress).isValidSignature.selector, + hash, + signature + ); + assembly { + let cdStart := add(callData, 32) + let success := staticcall( + gas, // forward all gas + walletAddress, // address of Wallet contract + cdStart, // pointer to start of input + mload(callData), // length of input + cdStart, // write output over input + 32 // output size is 32 bytes + ) + + switch success + case 0 { + // Revert with `Error("WALLET_ERROR")` + mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000) + mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000) + mstore(64, 0x0000000c57414c4c45545f4552524f5200000000000000000000000000000000) + mstore(96, 0) + revert(0, 100) + } + case 1 { + // Signature is valid if call did not revert and returned true + isValid := mload(cdStart) + } + } + return isValid; + } +} \ No newline at end of file diff --git a/contracts/staking/contracts/src/interfaces/IStructs.sol b/contracts/staking/contracts/src/interfaces/IStructs.sol index c47c998244..2d3fc0991c 100644 --- a/contracts/staking/contracts/src/interfaces/IStructs.sol +++ b/contracts/staking/contracts/src/interfaces/IStructs.sol @@ -21,6 +21,16 @@ pragma solidity ^0.5.5; interface IStructs { + // Allowed signature types. + enum SignatureType { + Illegal, // 0x00, default value + Invalid, // 0x01 + EIP712, // 0x02 + EthSign, // 0x03 + Wallet, // 0x04 + NSignatureTypes // 0x05, number of signature types. Always leave at end. + } + struct Timelock { uint64 lockedAt; uint96 total; diff --git a/contracts/staking/contracts/src/interfaces/IWallet.sol b/contracts/staking/contracts/src/interfaces/IWallet.sol new file mode 100644 index 0000000000..359ac371bc --- /dev/null +++ b/contracts/staking/contracts/src/interfaces/IWallet.sol @@ -0,0 +1,30 @@ +/* + Copyright 2018 ZeroEx Intl. + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +pragma solidity ^0.5.5; + + +interface IWallet /* is EIP-1271 */ { + + /// @dev Verifies that a signature is valid. + /// @param hash Message hash that is signed. + /// @param signature Proof of signing. + /// @return The function selector for this function + function isValidSignature( + bytes32 hash, + bytes calldata signature + ) + external + view + returns (bytes32 isValidSignatureSelector); +} \ No newline at end of file diff --git a/contracts/staking/contracts/src/vaults/RewardVault.sol b/contracts/staking/contracts/src/vaults/RewardVault.sol index cbf1a4d233..c6e994a102 100644 --- a/contracts/staking/contracts/src/vaults/RewardVault.sol +++ b/contracts/staking/contracts/src/vaults/RewardVault.sol @@ -31,6 +31,7 @@ contract RewardVault is MixinConstants, MixinVaultCore { + // @TODO -- ADD README's TO EACH DIRECTORY using LibSafeMath for uint256; using LibSafeMath96Bit for uint96; diff --git a/contracts/staking/contracts/src/wrappers/MixinPoolsWrapper.sol b/contracts/staking/contracts/src/wrappers/MixinPoolsWrapper.sol index 6b1f86c775..5891f5ed8e 100644 --- a/contracts/staking/contracts/src/wrappers/MixinPoolsWrapper.sol +++ b/contracts/staking/contracts/src/wrappers/MixinPoolsWrapper.sol @@ -46,6 +46,7 @@ contract MixinPoolsWrapper is function addMakerToPool( bytes32 poolId, address makerAddress, + bytes32 makerSignedHash, bytes calldata makerSignature ) external @@ -54,6 +55,7 @@ contract MixinPoolsWrapper is _addMakerToPool( poolId, makerAddress, + makerSignedHash, makerSignature, msg.sender ); diff --git a/contracts/staking/contracts/src/wrappers/MixinSignatureValidatorWrapper.sol b/contracts/staking/contracts/src/wrappers/MixinSignatureValidatorWrapper.sol new file mode 100644 index 0000000000..41dce33df5 --- /dev/null +++ b/contracts/staking/contracts/src/wrappers/MixinSignatureValidatorWrapper.sol @@ -0,0 +1,39 @@ +/* + Copyright 2018 ZeroEx Intl. + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +pragma solidity ^0.5.5; + +import "../core/MixinSignatureValidator.sol"; + + +contract MixinSignatureValidatorWrapper is + MixinSignatureValidator +{ + + /// @dev Verifies that a hash has been signed by the given signer. + /// @param hash Any 32 byte hash. + /// @param signerAddress Address that should have signed the given hash. + /// @param signature Proof that the hash has been signed by signer. + /// @return True if the address recovered from the provided signature matches the input signer address. + function isValidSignature( + bytes32 hash, + address signerAddress, + bytes memory signature + ) + internal + view + returns (bool isValid) + { + return _isValidSignature(hash, signerAddress, signature); + } +} \ No newline at end of file diff --git a/contracts/staking/test/core_test.ts b/contracts/staking/test/core_test.ts index d426624626..5fe184ec85 100644 --- a/contracts/staking/test/core_test.ts +++ b/contracts/staking/test/core_test.ts @@ -609,14 +609,14 @@ describe('Staking Core', () => { ]; await Promise.all([ // pool 0 - stakingWrapper.addMakerToPoolAsync(poolIds[0], makersByPoolId[0][0], "0x00", poolOperators[0]), + stakingWrapper.addMakerToPoolAsync(poolIds[0], makersByPoolId[0][0], "0x00", "0x00", poolOperators[0]), // pool 1 - stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][0], "0x00", poolOperators[1]), - stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][1], "0x00", poolOperators[1]), + stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][0], "0x00", "0x00", poolOperators[1]), + stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][1], "0x00", "0x00", poolOperators[1]), // pool 2 - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][0], "0x00", poolOperators[2]), - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][1], "0x00", poolOperators[2]), - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][2], "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][0], "0x00", "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][1], "0x00", "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][2], "0x00", "0x00", poolOperators[2]), ]); ///// 2 PAY FEES ///// await Promise.all([ @@ -787,7 +787,7 @@ describe('Staking Core', () => { // add maker to pool const makerAddress = makers[0]; const makerSignature = "0x"; - await stakingWrapper.addMakerToPoolAsync(poolId, makerAddress, makerSignature, operatorAddress); + await stakingWrapper.addMakerToPoolAsync(poolId, makerAddress, "0x00", makerSignature, operatorAddress); // check the pool id of the maker const poolIdOfMaker = await stakingWrapper.getMakerPoolId(makerAddress); expect(poolIdOfMaker).to.be.equal(poolId); @@ -796,7 +796,7 @@ describe('Staking Core', () => { expect(makerAddressesForPool).to.be.deep.equal([makerAddress]); // try to add the same maker address again await expectTransactionFailedAsync( - stakingWrapper.addMakerToPoolAsync(poolId, makerAddress, makerSignature, operatorAddress), + stakingWrapper.addMakerToPoolAsync(poolId, makerAddress, "0x00", makerSignature, operatorAddress), RevertReason.MakerAddressAlreadyRegistered ); // try to add a new maker address from an address other than the pool operator @@ -804,7 +804,7 @@ describe('Staking Core', () => { const anotherMakerAddress = makers[1]; const anotherMakerSignature = "0x"; await expectTransactionFailedAsync( - stakingWrapper.addMakerToPoolAsync(poolId, anotherMakerAddress, anotherMakerSignature, notOperatorAddress), + stakingWrapper.addMakerToPoolAsync(poolId, anotherMakerAddress, "0x00", anotherMakerSignature, notOperatorAddress), RevertReason.OnlyCallableByPoolOperator ); // try to remove the maker address from an address other than the operator @@ -860,14 +860,14 @@ describe('Staking Core', () => { ]; await Promise.all([ // pool 0 - stakingWrapper.addMakerToPoolAsync(poolIds[0], makersByPoolId[0][0], "0x00", poolOperators[0]), + stakingWrapper.addMakerToPoolAsync(poolIds[0], makersByPoolId[0][0], "0x00", "0x00", poolOperators[0]), // pool 1 - stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][0], "0x00", poolOperators[1]), - stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][1], "0x00", poolOperators[1]), + stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][0], "0x00", "0x00", poolOperators[1]), + stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][1], "0x00", "0x00", poolOperators[1]), // pool 2 - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][0], "0x00", poolOperators[2]), - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][1], "0x00", poolOperators[2]), - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][2], "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][0], "0x00", "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][1], "0x00", "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][2], "0x00", "0x00", poolOperators[2]), ]); ///// 2 PAY FEES ///// await Promise.all([ @@ -1008,14 +1008,14 @@ describe('Staking Core', () => { ]; await Promise.all([ // pool 0 - stakingWrapper.addMakerToPoolAsync(poolIds[0], makersByPoolId[0][0], "0x00", poolOperators[0]), + stakingWrapper.addMakerToPoolAsync(poolIds[0], makersByPoolId[0][0], "0x00", "0x00", poolOperators[0]), // pool 1 - stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][0], "0x00", poolOperators[1]), - stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][1], "0x00", poolOperators[1]), + stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][0], "0x00", "0x00", poolOperators[1]), + stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][1], "0x00", "0x00", poolOperators[1]), // pool 2 - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][0], "0x00", poolOperators[2]), - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][1], "0x00", poolOperators[2]), - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][2], "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][0], "0x00", "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][1], "0x00", "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][2], "0x00", "0x00", poolOperators[2]), ]); ///// 2 PAY FEES ///// await Promise.all([ @@ -1252,14 +1252,14 @@ describe('Staking Core', () => { ]; await Promise.all([ // pool 0 - stakingWrapper.addMakerToPoolAsync(poolIds[0], makersByPoolId[0][0], "0x00", poolOperators[0]), + stakingWrapper.addMakerToPoolAsync(poolIds[0], makersByPoolId[0][0], "0x00", "0x00", poolOperators[0]), // pool 1 - stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][0], "0x00", poolOperators[1]), - stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][1], "0x00", poolOperators[1]), + stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][0], "0x00", "0x00", poolOperators[1]), + stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][1], "0x00", "0x00", poolOperators[1]), // pool 2 - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][0], "0x00", poolOperators[2]), - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][1], "0x00", poolOperators[2]), - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][2], "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][0], "0x00", "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][1], "0x00", "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][2], "0x00", "0x00", poolOperators[2]), ]); ///// 2 PAY FEES ///// await Promise.all([ @@ -1438,14 +1438,14 @@ describe('Staking Core', () => { ]; await Promise.all([ // pool 0 - stakingWrapper.addMakerToPoolAsync(poolIds[0], makersByPoolId[0][0], "0x00", poolOperators[0]), + stakingWrapper.addMakerToPoolAsync(poolIds[0], makersByPoolId[0][0], "0x00", "0x00", poolOperators[0]), // pool 1 - stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][0], "0x00", poolOperators[1]), - stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][1], "0x00", poolOperators[1]), + stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][0], "0x00", "0x00", poolOperators[1]), + stakingWrapper.addMakerToPoolAsync(poolIds[1], makersByPoolId[1][1], "0x00", "0x00", poolOperators[1]), // pool 2 - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][0], "0x00", poolOperators[2]), - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][1], "0x00", poolOperators[2]), - stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][2], "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][0], "0x00", "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][1], "0x00", "0x00", poolOperators[2]), + stakingWrapper.addMakerToPoolAsync(poolIds[2], makersByPoolId[2][2], "0x00", "0x00", poolOperators[2]), ]); ///// 2 PAY FEES ///// await Promise.all([ diff --git a/contracts/staking/test/utils/staking_wrapper.ts b/contracts/staking/test/utils/staking_wrapper.ts index 0886e00ca2..84b6c5d5b5 100644 --- a/contracts/staking/test/utils/staking_wrapper.ts +++ b/contracts/staking/test/utils/staking_wrapper.ts @@ -251,8 +251,8 @@ export class StakingWrapper { const poolId = (createPoolLog as any).args.poolId; return poolId; } - public async addMakerToPoolAsync(poolId: string, makerAddress: string, makerSignature: string, operatorAddress: string): Promise { - const calldata = this.getStakingContract().addMakerToPool.getABIEncodedTransactionData(poolId, makerAddress, makerSignature); + public async addMakerToPoolAsync(poolId: string, makerAddress: string, makerSignedHash: string, makerSignature: string, operatorAddress: string): Promise { + const calldata = this.getStakingContract().addMakerToPool.getABIEncodedTransactionData(poolId, makerAddress, makerSignedHash, makerSignature); const txReceipt = await this._executeTransactionAsync(calldata, operatorAddress); return txReceipt; }