From 4e020fafccfd386f03f64f96a772a294c6ad6dbf Mon Sep 17 00:00:00 2001 From: Paruyr Gevorgyan Date: Wed, 16 Jan 2019 17:07:11 +0100 Subject: [PATCH] Applied review comments from ben-review session #1 --- contracts/PriceOracleInterface.sol | 12 +++- contracts/PricerRule.sol | 27 +++++++- contracts/TokenHolder.sol | 69 +++++++++++++------ .../pricer_rule/PriceOracleFake.sol | 18 ++++- test/pricer_rule/add_price_oracle.js | 31 ++++++++- test/pricer_rule/constructor.js | 9 +++ test/pricer_rule/pay.js | 55 +++++++++++---- test/pricer_rule/utils.js | 5 ++ test/token_holder/execute_redemption.js | 54 +++++++++++---- test/token_holder/execute_rule.js | 56 +++++++++++---- 10 files changed, 268 insertions(+), 68 deletions(-) diff --git a/contracts/PriceOracleInterface.sol b/contracts/PriceOracleInterface.sol index 9266ea0..e87ec18 100644 --- a/contracts/PriceOracleInterface.sol +++ b/contracts/PriceOracleInterface.sol @@ -46,8 +46,16 @@ interface PriceOracleInterface { returns (bytes3); /** - * @notice Returns an amount of the quote currency needed to purchase - * one unit of the base currency. + * @notice Returns quote currency decimals. + */ + function decimals() + external + view + returns (uint8); + + /** + * @notice Returns an amount of the quote currency (see decimals()) needed + * to purchase one unit of the base currency. * * @dev Function throws an exception if the price is invalid, for example, * was not set, or became outdated, etc. diff --git a/contracts/PricerRule.sol b/contracts/PricerRule.sol index b6b7051..9e80e2d 100644 --- a/contracts/PricerRule.sol +++ b/contracts/PricerRule.sol @@ -70,6 +70,11 @@ contract PricerRule is Organized { */ uint256 public conversionRateDecimalsFromBaseCurrencyToToken; + /** + * @dev Required decimals for price oracles. + */ + uint8 public requiredPriceOracleDecimals; + /** * @dev Token rules address of the economy. */ @@ -111,6 +116,7 @@ contract PricerRule is Organized { * to the token. * @param _conversionRateDecimals The conversion rate's decimals from the * economy base currency to the token. + * @param _requiredPriceOracleDecimals Required decimals for price oracles. * @param _tokenRules The economy token rules address. */ constructor( @@ -119,6 +125,7 @@ contract PricerRule is Organized { bytes3 _baseCurrencyCode, uint256 _conversionRate, uint8 _conversionRateDecimals, + uint8 _requiredPriceOracleDecimals, address _tokenRules ) Organized(_organization) @@ -149,6 +156,8 @@ contract PricerRule is Organized { conversionRateDecimalsFromBaseCurrencyToToken = _conversionRateDecimals; + requiredPriceOracleDecimals = _requiredPriceOracleDecimals; + tokenRules = TokenRules(_tokenRules); } @@ -189,10 +198,19 @@ contract PricerRule is Organized { "'to' and 'amount' transfer arrays' lengths are not equal." ); + if (_toList.length == 0) { + return; + } + uint256 baseCurrencyCurrentPrice = baseCurrencyPrice( _payCurrencyCode ); + require( + baseCurrencyCurrentPrice != 0, + "Base currency price in pay currency is 0." + ); + require( isPriceInRange( _baseCurrencyIntendedPrice, @@ -206,7 +224,7 @@ contract PricerRule is Organized { for(uint256 i = 0; i < _amountList.length; ++i) { convertedAmounts[i] = convertPayCurrencyToToken( - _baseCurrencyIntendedPrice, + baseCurrencyCurrentPrice, _amountList[i] ); } @@ -228,6 +246,8 @@ contract PricerRule is Organized { * equal to the economy base currency code specified in this * contract constructor. * - The proposed price oracle does not exist. + * - The proposed price oracle decimals number is equal to + * the contract required price oracle decimals number. * * @param _priceOracle The proposed price oracle. */ @@ -242,6 +262,11 @@ contract PricerRule is Organized { "Price oracle address is null." ); + require( + _priceOracle.decimals() == requiredPriceOracleDecimals, + "Price oracle decimals number is difference from the required one." + ); + bytes3 payCurrencyCode = _priceOracle.quoteCurrency(); require( diff --git a/contracts/TokenHolder.sol b/contracts/TokenHolder.sol index 56c4362..7419292 100644 --- a/contracts/TokenHolder.sol +++ b/contracts/TokenHolder.sol @@ -45,21 +45,35 @@ contract TokenHolder { address _sessionKey ); + /** + * @dev Rule's hash is calculated by: + * keccak256( + * abi.encodePacked( + * to, // rule's address + * keccak(data), // data is rule's payload + * nonce, // non-used nonce for the specific session key + * v, r, s // signature + * ) + * ) + */ event RuleExecuted( - address indexed _to, - bytes4 _functionSelector, - address _sessionKey, - uint256 _nonce, - bytes32 _messageHash, + bytes32 _ruleHash, bool _status ); + /** + * @dev Redemption's hash is calculated by: + * keccak256( + * abi.encodePacked( + * to, // cogateway's address + * keccak(data), // data is cogateway::redeem function's payload + * nonce, // non-used nonce for the specific session key + * v, r, s // signature + * ) + * ) + */ event RedeemExecuted( - address indexed _to, - bytes4 _functionSelector, - address _sessionKey, - uint256 _nonce, - bytes32 _messageHash, + bytes32 _redemptionHash, bool _status ); @@ -349,14 +363,19 @@ contract TokenHolder { TokenRules(tokenRules).disallowTransfers(); - bytes4 functionSelector = bytesToBytes4(_data); + bytes32 ruleHash = keccak256( + abi.encodePacked( + _to, + keccak256(_data), + _nonce, + _v, + _r, + _s + ) + ); emit RuleExecuted( - _to, - functionSelector, - sessionKey, - _nonce, - messageHash, + ruleHash, executionStatus_ ); } @@ -406,15 +425,21 @@ contract TokenHolder { // solium-disable-next-line security/no-call-value (executionStatus_, returnData) = _to.call.value(msg.value)(_data); - token.approve(_to, 0); + bytes32 redemptionHash = keccak256( + abi.encodePacked( + _to, + keccak256(_data), + _nonce, + _v, + _r, + _s + ) + ); + emit RedeemExecuted( - _to, - functionSelector, - sessionKey, - _nonce, - messageHash, + redemptionHash, executionStatus_ ); } diff --git a/contracts/test_doubles/unit_tests/pricer_rule/PriceOracleFake.sol b/contracts/test_doubles/unit_tests/pricer_rule/PriceOracleFake.sol index 06f5880..9cda12b 100644 --- a/contracts/test_doubles/unit_tests/pricer_rule/PriceOracleFake.sol +++ b/contracts/test_doubles/unit_tests/pricer_rule/PriceOracleFake.sol @@ -28,12 +28,15 @@ contract PriceOracleFake is PriceOracleInterface { uint256 expirationHeight; + uint8 quoteCurrencyDecimals; + /* Special */ constructor( bytes3 _baseCurrencyCode, bytes3 _quoteCurrencyCode, + uint8 _quoteCurrencyDecimals, uint256 _initialPrice, uint256 _expirationHeight ) @@ -53,6 +56,8 @@ contract PriceOracleFake is PriceOracleInterface { quoteCurrencyCode = _quoteCurrencyCode; + quoteCurrencyDecimals = _quoteCurrencyDecimals; + setPrice(_initialPrice, _expirationHeight); } @@ -85,6 +90,17 @@ contract PriceOracleFake is PriceOracleInterface { return quoteCurrencyCode; } + /** + * @notice Returns quote currency decimals. + */ + function decimals() + external + view + returns(uint8) + { + return quoteCurrencyDecimals; + } + /** * @notice Returns an amount of the quote currency needed to purchase * one unit of the base currency. @@ -117,8 +133,6 @@ contract PriceOracleFake is PriceOracleInterface { ) public { - require(_price != 0, "Price is 0."); - require( _expirationHeight > block.number, "Price expiration height is lte to the current block height." diff --git a/test/pricer_rule/add_price_oracle.js b/test/pricer_rule/add_price_oracle.js index ffd22e6..bb70351 100644 --- a/test/pricer_rule/add_price_oracle.js +++ b/test/pricer_rule/add_price_oracle.js @@ -58,11 +58,38 @@ contract('PricerRule::add_price_oracle', async () => { ); }); + it('Reverts as the proposed price oracle decimals number is invalid.', async () => { + const { + organizationWorker, + baseCurrencyCode, + requiredPriceOracleDecimals, + pricerRule, + } = await PricerRuleUtils.createTokenEconomy(accountProvider); + + const priceOracle = await PriceOracleFake.new( + web3.utils.stringToHex(baseCurrencyCode), + web3.utils.stringToHex('BTC'), + requiredPriceOracleDecimals + 1, + 1, // price oracle initial price + (await web3.eth.getBlockNumber()) + 10000, // expiration height + ); + + await Utils.expectRevert( + pricerRule.addPriceOracle( + priceOracle.address, + { from: organizationWorker }, + ), + 'Should revert as the proposed price oracle decimals number is invalid.', + 'Price oracle decimals number is difference from the required one.', + ); + }); + it('Reverts as the proposed price oracle base currency code does not ' + 'match with pricer base currency.', async () => { const { organizationWorker, baseCurrencyCode, + requiredPriceOracleDecimals, pricerRule, quoteCurrencyCode, } = await PricerRuleUtils.createTokenEconomy(accountProvider); @@ -78,6 +105,7 @@ contract('PricerRule::add_price_oracle', async () => { const priceOracle = await PriceOracleFake.new( web3.utils.stringToHex(anotherBaseCurrencyCode), web3.utils.stringToHex(quoteCurrencyCode), + requiredPriceOracleDecimals, 100, // initial price (await web3.eth.getBlockNumber()) + 10000, // expiration height ); @@ -97,6 +125,7 @@ contract('PricerRule::add_price_oracle', async () => { const { organizationWorker, baseCurrencyCode, + requiredPriceOracleDecimals, pricerRule, priceOracle, quoteCurrencyCode, @@ -110,6 +139,7 @@ contract('PricerRule::add_price_oracle', async () => { const priceOracle2 = await PriceOracleFake.new( web3.utils.stringToHex(baseCurrencyCode), web3.utils.stringToHex(quoteCurrencyCode), + requiredPriceOracleDecimals, 100, // initial price (await web3.eth.getBlockNumber()) + 10000, // expiration height ); @@ -140,7 +170,6 @@ contract('PricerRule::add_price_oracle', async () => { { from: organizationWorker }, ); - const events = Event.decodeTransactionResponse( response, ); diff --git a/test/pricer_rule/constructor.js b/test/pricer_rule/constructor.js index 85eb8a6..37c5267 100644 --- a/test/pricer_rule/constructor.js +++ b/test/pricer_rule/constructor.js @@ -35,6 +35,7 @@ contract('PricerRule::constructor', async () => { web3.utils.stringToHex('OST'), // base currency code 1, // conversion rate 0, // conversion rate decimals + 0, // price oracles required decimals number accountProvider.get(), // token rules ), 'Should revert as the economy token address is null.', @@ -54,6 +55,7 @@ contract('PricerRule::constructor', async () => { web3.utils.stringToHex(''), // base currency code 1, // conversion rate 0, // conversion rate decimals + 0, // price oracles required decimals number accountProvider.get(), // token rules ), 'Should revert as the base currency code is null.', @@ -73,6 +75,7 @@ contract('PricerRule::constructor', async () => { web3.utils.stringToHex('OST'), // base currency code 0, // conversion rate 0, // conversion rate decimals + 0, // price oracles required decimals number accountProvider.get(), // token rules ), 'Should revert as the conversion rate from the base currency to the token is 0.', @@ -92,6 +95,7 @@ contract('PricerRule::constructor', async () => { web3.utils.stringToHex('OST'), // base currency code 1, // conversion rate 0, // conversion rate decimals + 0, // price oracles required decimals number Utils.NULL_ADDRESS, // token rules ), 'Should revert as token rules is null.', @@ -116,6 +120,7 @@ contract('PricerRule::constructor', async () => { web3.utils.stringToHex('OST'), // base currency code 10, // conversion rate 1, // conversion rate decimals + 2, // price oracles required decimals number tokenRules, // token rules ); @@ -147,6 +152,10 @@ contract('PricerRule::constructor', async () => { (await pricerRule.conversionRateDecimalsFromBaseCurrencyToToken.call()).eqn(1), ); + assert.isOk( + (await pricerRule.requiredPriceOracleDecimals.call()).eqn(2), + ); + assert.strictEqual( (await pricerRule.tokenRules.call()), tokenRules, diff --git a/test/pricer_rule/pay.js b/test/pricer_rule/pay.js index 0fb88ab..2d70c1e 100644 --- a/test/pricer_rule/pay.js +++ b/test/pricer_rule/pay.js @@ -17,7 +17,6 @@ const web3 = require('../test_lib/web3.js'); const Utils = require('../test_lib/utils'); const PricerRuleUtils = require('./utils.js'); const { AccountProvider } = require('../test_lib/utils'); -const { Event } = require('../test_lib/event_decoder'); async function prepare(accountProvider) { const r = await PricerRuleUtils.createTokenEconomy(accountProvider); @@ -33,7 +32,7 @@ async function prepare(accountProvider) { { from: r.fromAddress }, ); - r.spendingLimit = 111; + r.spendingLimit = 1000000; r.token.approve( r.tokenRules.address, @@ -65,8 +64,8 @@ contract('PricerRule::pay', async () => { await Utils.expectRevert( pricerRule.pay( Utils.NULL_ADDRESS, - [], // 'to' addresses - [], // amounts + [accountProvider.get()], // 'to' addresses + [1], // amounts web3.utils.stringToHex(quoteCurrencyCode), priceOracleInitialPrice, { from: accountProvider.get() }, @@ -191,6 +190,34 @@ contract('PricerRule::pay', async () => { 'Price expiration height is lte to the current block height.', ); }); + + it('Reverts if an price oracle returns 0 price.', async () => { + const { + pricerRule, + quoteCurrencyCode, + priceOracleInitialPrice, + priceOracle, + fromAddress, + } = await prepare(accountProvider); + + await priceOracle.setPrice( + 0, + (await web3.eth.getBlockNumber()) + 10000, + ); + + await Utils.expectRevert( + pricerRule.pay( + fromAddress, + [accountProvider.get()], // 'to' addresses + [2], // amounts + web3.utils.stringToHex(quoteCurrencyCode), + priceOracleInitialPrice, + { from: accountProvider.get() }, + ), + 'Should revert as the price oracle returns 0 as price.', + 'Base currency price in pay currency is 0.', + ); + }); }); contract('Positive Paths', async (accounts) => { @@ -202,33 +229,33 @@ contract('PricerRule::pay', async () => { tokenRules, conversionRate, conversionRateDecimals, + requiredPriceOracleDecimals, pricerRule, quoteCurrencyCode, - priceOracleInitialPrice, priceOracle, fromAddress, } = await prepare(accountProvider); - const oraclePrice = 2; + const oraclePriceBN = new BN(4 * (10 ** requiredPriceOracleDecimals)); await priceOracle.setPrice( - oraclePrice, + oraclePriceBN.toNumber(), (await web3.eth.getBlockNumber()) + 10000, ); - const acceptanceMargin = 5; + const acceptanceMarginBN = new BN(3 * (10 ** requiredPriceOracleDecimals)); await pricerRule.setAcceptanceMargin( web3.utils.stringToHex(quoteCurrencyCode), - acceptanceMargin, + acceptanceMarginBN.toNumber(), { from: organizationWorker }, ); const to1 = accountProvider.get(); const to2 = accountProvider.get(); - const amount1BN = new BN(11 * (10 ** 12)); - const amount2BN = new BN(7 * (10 ** 12)); + const amount1BN = new BN(11 * (10 ** requiredPriceOracleDecimals)); + const amount2BN = new BN(7 * (10 ** requiredPriceOracleDecimals)); - const intendedPriceBN = new BN(oraclePrice + acceptanceMargin); + const intendedPriceBN = oraclePriceBN.add(acceptanceMarginBN); await pricerRule.pay( fromAddress, @@ -240,10 +267,10 @@ contract('PricerRule::pay', async () => { ); const convertedAmount1BN = convertPayCurrencyToToken( - amount1BN, intendedPriceBN, new BN(conversionRate), new BN(conversionRateDecimals), + amount1BN, oraclePriceBN, new BN(conversionRate), new BN(conversionRateDecimals), ); const convertedAmount2BN = convertPayCurrencyToToken( - amount2BN, intendedPriceBN, new BN(conversionRate), new BN(conversionRateDecimals), + amount2BN, oraclePriceBN, new BN(conversionRate), new BN(conversionRateDecimals), ); diff --git a/test/pricer_rule/utils.js b/test/pricer_rule/utils.js index 28f5ca3..48a4f0a 100644 --- a/test/pricer_rule/utils.js +++ b/test/pricer_rule/utils.js @@ -75,12 +75,15 @@ module.exports.createTokenEconomy = async (accountProvider) => { const conversionRateDecimals = 2; + const requiredPriceOracleDecimals = 7; + const pricerRule = await PricerRule.new( organization.address, token.address, web3.utils.stringToHex(baseCurrencyCode.toString()), conversionRate, conversionRateDecimals, + requiredPriceOracleDecimals, tokenRules.address, ); @@ -91,6 +94,7 @@ module.exports.createTokenEconomy = async (accountProvider) => { const priceOracle = await PriceOracleFake.new( web3.utils.stringToHex(baseCurrencyCode), web3.utils.stringToHex(quoteCurrencyCode), + requiredPriceOracleDecimals, priceOracleInitialPrice, initialPriceExpirationHeight, ); @@ -104,6 +108,7 @@ module.exports.createTokenEconomy = async (accountProvider) => { baseCurrencyCode, conversionRate, conversionRateDecimals, + requiredPriceOracleDecimals, pricerRule, quoteCurrencyCode, priceOracleInitialPrice, diff --git a/test/token_holder/execute_redemption.js b/test/token_holder/execute_redemption.js index 983bcb3..b4e4d79 100644 --- a/test/token_holder/execute_redemption.js +++ b/test/token_holder/execute_redemption.js @@ -27,6 +27,32 @@ const sessionPrivateKey1 = '0xa8225c01ceeaf01d7bc7c1b1b929037bd4050967c5730c0b85 // const sessionPublicKey2 = '0xBbfd1BF77dA692abc82357aC001415b98d123d17'; const sessionPrivateKey2 = '0x6817f551bbc3e12b8fe36787ab192c921390d6176a3324ed02f96935a370bc41'; +function generateRedemptionHash( + to, data, nonce, v, r, s, +) { + return web3.utils.soliditySha3( + { + t: 'address', v: to, + }, + { + t: 'bytes32', v: web3.utils.keccak256(data), + }, + { + t: 'uint256', v: nonce, + }, + { + t: 'uint8', v, + }, + { + t: 'bytes32', v: r, + }, + { + t: 'bytes32', v: s, + }, + + ); +} + function generateTokenHolderexecuteRedemptionFunctionCallPrefix() { return web3.eth.abi.encodeFunctionSignature({ name: 'executeRedemption', @@ -512,7 +538,6 @@ contract('TokenHolder::redeem', async (accounts) => { const { coGatewayRedeemFunctionData, - exTxHash, exTxSignature, } = await generateCoGatewayRedeemFunctionExTx( tokenHolder, @@ -542,11 +567,14 @@ contract('TokenHolder::redeem', async (accounts) => { Event.assertEqual(events[0], { name: 'RedeemExecuted', args: { - _to: coGateway.address, - _functionSelector: await tokenHolder.COGATEWAY_REDEEM_CALLPREFIX.call(), - _sessionKey: sessionPublicKey1, - _nonce: new BN(tokenHolderNonce), - _messageHash: exTxHash, + _redemptionHash: generateRedemptionHash( + coGateway.address, + coGatewayRedeemFunctionData, + redeemerNonce, + exTxSignature.v, + EthUtils.bufferToHex(exTxSignature.r), + EthUtils.bufferToHex(exTxSignature.s), + ), _status: true, }, }); @@ -574,7 +602,6 @@ contract('TokenHolder::redeem', async (accounts) => { const { coGatewayRedeemFunctionData, - exTxHash, exTxSignature, } = await generateCoGatewayRedeemFunctionExTx( tokenHolder, @@ -606,11 +633,14 @@ contract('TokenHolder::redeem', async (accounts) => { Event.assertEqual(events[0], { name: 'RedeemExecuted', args: { - _to: coGateway.address, - _functionSelector: await tokenHolder.COGATEWAY_REDEEM_CALLPREFIX.call(), - _sessionKey: sessionPublicKey1, - _nonce: new BN(tokenHolderNonce), - _messageHash: exTxHash, + _redemptionHash: generateRedemptionHash( + coGateway.address, + coGatewayRedeemFunctionData, + redeemerNonce, + exTxSignature.v, + EthUtils.bufferToHex(exTxSignature.r), + EthUtils.bufferToHex(exTxSignature.s), + ), _status: false, }, }); diff --git a/test/token_holder/execute_rule.js b/test/token_holder/execute_rule.js index f3cc7a1..04b2959 100644 --- a/test/token_holder/execute_rule.js +++ b/test/token_holder/execute_rule.js @@ -27,6 +27,32 @@ const sessionPrivateKey1 = '0xa8225c01ceeaf01d7bc7c1b1b929037bd4050967c5730c0b85 const sessionPublicKey2 = '0xBbfd1BF77dA692abc82357aC001415b98d123d17'; const sessionPrivateKey2 = '0x6817f551bbc3e12b8fe36787ab192c921390d6176a3324ed02f96935a370bc41'; +function generateRuleHash( + to, data, nonce, v, r, s, +) { + return web3.utils.soliditySha3( + { + t: 'address', v: to, + }, + { + t: 'bytes32', v: web3.utils.keccak256(data), + }, + { + t: 'uint256', v: nonce, + }, + { + t: 'uint8', v, + }, + { + t: 'bytes32', v: r, + }, + { + t: 'bytes32', v: s, + }, + + ); +} + function generateTokenHolderAuthorizeSessionFunctionData( sessionKey, spendingLimit, expirationHeight, ) { @@ -604,7 +630,6 @@ contract('TokenHolder::executeRule', async () => { const { mockRulePassFunctionData, - exTxHash, exTxSignature, } = await generateMockRulePassFunctionExTx( tokenHolder, nonce, sessionPrivateKey1, @@ -629,16 +654,17 @@ contract('TokenHolder::executeRule', async () => { 1, ); - const passCallPrefix = await mockRule.PASS_CALLPREFIX.call(); - Event.assertEqual(events[0], { name: 'RuleExecuted', args: { - _to: mockRule.address, - _functionSelector: passCallPrefix, - _sessionKey: sessionPublicKey1, - _nonce: new BN(nonce), - _messageHash: exTxHash, + _ruleHash: generateRuleHash( + mockRule.address, + mockRulePassFunctionData, + nonce, + exTxSignature.v, + EthUtils.bufferToHex(exTxSignature.r), + EthUtils.bufferToHex(exTxSignature.s), + ), _status: true, }, }); @@ -662,7 +688,6 @@ contract('TokenHolder::executeRule', async () => { const { mockRuleFailFunctionData, - exTxHash, exTxSignature, } = await generateMockRuleFailFunctionExTx( tokenHolder, nonce, sessionPrivateKey1, @@ -690,11 +715,14 @@ contract('TokenHolder::executeRule', async () => { Event.assertEqual(events[0], { name: 'RuleExecuted', args: { - _to: mockRule.address, - _functionSelector: await mockRule.FAIL_CALLPREFIX.call(), - _sessionKey: sessionPublicKey1, - _nonce: new BN(nonce), - _messageHash: exTxHash, + _ruleHash: generateRuleHash( + mockRule.address, + mockRuleFailFunctionData, + nonce, + exTxSignature.v, + EthUtils.bufferToHex(exTxSignature.r), + EthUtils.bufferToHex(exTxSignature.s), + ), _status: false, }, });