Skip to content

Commit

Permalink
sip 98 double swing trade exchange fees (#935)
Browse files Browse the repository at this point in the history 
* update calculation of exchangeFeeRate for swing trades from long <-> short synths without sUSD step

* fix audit findings on missing exchangeFeeRate assignment and update Exchanger tests
  • Loading branch information
@jacko125
jacko125 committed Nov 30, 2020
1 parent f33ed87 commit 3b03563
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 22 deletions.
27 changes: 22 additions & 5 deletions contracts/Exchanger.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -808,11 +808,28 @@ contract Exchanger is Owned, MixinResolver, MixinSystemSettings, IExchanger {
exchangeFeeRate = _feeRateForExchange(sourceCurrencyKey, destinationCurrencyKey);
}

function _feeRateForExchange(
bytes32, // API for source in case pricing model evolves to include source rate /* sourceCurrencyKey */
bytes32 destinationCurrencyKey
) internal view returns (uint exchangeFeeRate) {
return getExchangeFeeRate(destinationCurrencyKey);
function _feeRateForExchange(bytes32 sourceCurrencyKey, bytes32 destinationCurrencyKey)
internal
view
returns (uint exchangeFeeRate)
{
// Get the exchange fee rate as per destination currencyKey
exchangeFeeRate = getExchangeFeeRate(destinationCurrencyKey);

if (sourceCurrencyKey == sUSD || destinationCurrencyKey == sUSD) {
return exchangeFeeRate;
}

// Is this a swing trade? long to short or short to long skipping sUSD.
if (
(sourceCurrencyKey[0] == 0x73 && destinationCurrencyKey[0] == 0x69) ||
(sourceCurrencyKey[0] == 0x69 && destinationCurrencyKey[0] == 0x73)
) {
// Double the exchange fee
exchangeFeeRate = exchangeFeeRate.mul(2);
}

return exchangeFeeRate;
}

function getAmountsForExchange(
Expand Down
78 changes: 61 additions & 17 deletions test/contracts/Exchanger.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ const {
defaults: { WAITING_PERIOD_SECS, PRICE_DEVIATION_THRESHOLD_FACTOR },
} = require('../..');

const BN = require('bn.js');

const bnCloseVariance = '30';

const MockAggregator = artifacts.require('MockAggregatorV2V3');
Expand Down Expand Up @@ -390,15 +392,31 @@ contract('Exchanger (spec tests)', async accounts => {
actualFeeRate = await exchanger.feeRateForExchange(sUSD, iBTC);
assert.bnEqual(actualFeeRate, exchangeFeeRate, 'Rate must be the exchange fee rate');
});
it('for an inverse synth and a long synth, returns regular exchange fee', async () => {
it('for an inverse synth and a long synth, returns double the regular exchange fee', async () => {
let actualFeeRate = await exchanger.feeRateForExchange(iBTC, sEUR);
assert.bnEqual(actualFeeRate, exchangeFeeRate, 'Rate must be the exchange fee rate');
assert.bnEqual(
actualFeeRate,
exchangeFeeRate.mul(new BN(2)),
'Rate must be the exchange fee rate'
);
actualFeeRate = await exchanger.feeRateForExchange(sEUR, iBTC);
assert.bnEqual(actualFeeRate, exchangeFeeRate, 'Rate must be the exchange fee rate');
assert.bnEqual(
actualFeeRate,
exchangeFeeRate.mul(new BN(2)),
'Rate must be the exchange fee rate'
);
actualFeeRate = await exchanger.feeRateForExchange(sBTC, iBTC);
assert.bnEqual(actualFeeRate, exchangeFeeRate, 'Rate must be the exchange fee rate');
assert.bnEqual(
actualFeeRate,
exchangeFeeRate.mul(new BN(2)),
'Rate must be the exchange fee rate'
);
actualFeeRate = await exchanger.feeRateForExchange(iBTC, sBTC);
assert.bnEqual(actualFeeRate, exchangeFeeRate, 'Rate must be the exchange fee rate');
assert.bnEqual(
actualFeeRate,
exchangeFeeRate.mul(new BN(2)),
'Rate must be the exchange fee rate'
);
});
});

Expand Down Expand Up @@ -2091,19 +2109,14 @@ contract('Exchanger (spec tests)', async accounts => {
const assertExchangeSucceeded = async ({
amountExchanged,
txn,
exchangeFeeRateMultiplier = 1,
from = sUSD,
to = iBTC,
toContract = iBTCContract,
prevBalance,
}) => {
// Note: this presumes balance was empty before the exchange - won't work when
// exchanging into sUSD as there is an existing sUSD balance from minting
const exchangeFeeRate = await exchanger.feeRateForExchange(sUSD, iBTC);
const actualExchangeFee = multiplyDecimal(
exchangeFeeRate,
toUnit(exchangeFeeRateMultiplier)
);
const actualExchangeFee = await exchanger.feeRateForExchange(from, to);
const balance = await toContract.balanceOf(account1);
const effectiveValue = await exchangeRates.effectiveValue(
from,
Expand Down Expand Up @@ -2234,11 +2247,17 @@ contract('Exchanger (spec tests)', async accounts => {
from: account1,
});
});
it('then it exchanges correctly from iBTC to sBTC, not doubling the fee', async () => {
it('then it exchanges correctly from iBTC to sBTC, doubling the fee based on the destination Synth', async () => {
// get exchange fee for sUSD to sBTC (Base rate for destination synth)
const baseExchangeRate = await exchanger.feeRateForExchange(sUSD, sBTC);
const expectedExchangeRate = await exchanger.feeRateForExchange(iBTC, sBTC);

// swing trade should be double the base exchange fee
assert.bnEqual(expectedExchangeRate, baseExchangeRate.mul(new BN(2)));

await assertExchangeSucceeded({
amountExchanged: iBTCexchangeAmount,
txn,
exchangeFeeRateMultiplier: 1,
from: iBTC,
to: sBTC,
toContract: sBTCContract,
Expand All @@ -2252,11 +2271,20 @@ contract('Exchanger (spec tests)', async accounts => {
from: account1,
});
});
it('then it exchanges correctly from iBTC to sEUR, not doubling the fee', async () => {
it('then it exchanges correctly from iBTC to sEUR, doubling the fee', async () => {
// get exchange fee for sUSD to sEUR (Base rate for destination synth)
const baseExchangeRate = await exchanger.feeRateForExchange(sUSD, sEUR);
const expectedExchangeRate = await exchanger.feeRateForExchange(
iBTC,
sEUR
);

// swing trade should be double the base exchange fee
assert.bnEqual(expectedExchangeRate, baseExchangeRate.mul(new BN(2)));

await assertExchangeSucceeded({
amountExchanged: iBTCexchangeAmount,
txn,
exchangeFeeRateMultiplier: 1,
from: iBTC,
to: sEUR,
toContract: sEURContract,
Expand All @@ -2273,11 +2301,20 @@ contract('Exchanger (spec tests)', async accounts => {
from: account1,
});
});
it('then it exchanges correctly from sEUR to iBTC, not doubling the fee', async () => {
it('then it exchanges correctly from sEUR to iBTC, doubling the fee', async () => {
// get exchange fee for sUSD to iBTC (Base rate for destination synth)
const baseExchangeRate = await exchanger.feeRateForExchange(sUSD, iBTC);
const expectedExchangeRate = await exchanger.feeRateForExchange(
sEUR,
iBTC
);

// swing trade should be double the base exchange fee
assert.bnEqual(expectedExchangeRate, baseExchangeRate.mul(new BN(2)));

await assertExchangeSucceeded({
amountExchanged: sEURExchangeAmount,
txn,
exchangeFeeRateMultiplier: 1,
from: sEUR,
to: iBTC,
toContract: iBTCContract,
Expand All @@ -2299,6 +2336,13 @@ contract('Exchanger (spec tests)', async accounts => {
});
});
it('then it exchanges correctly out of iBTC, with the regular fee', async () => {
// get exchange fee for sETH to sUSD (Base rate for destination synth into sUSD)
const baseExchangeRate = await exchanger.feeRateForExchange(sETH, sUSD);
const expectedExchangeRate = await exchanger.feeRateForExchange(iBTC, sUSD);

// exchange fee should be the same base exchange fee
assert.bnEqual(expectedExchangeRate, baseExchangeRate);

await assertExchangeSucceeded({
amountExchanged: iBTCexchangeAmount,
txn,
Expand Down

0 comments on commit 3b03563

Please sign in to comment.