Skip to content

Commit e3050fd

Browse files
davidvuong0xjocke
authored and
sunnyvempati
committed
L-13, L-12, L-11, L-09 (#2209)
* fix: L-13 * fix: L-12 * fix: L-11 * fix: L-09 * Update markets/bfp-market/contracts/storage/Position.sol Co-authored-by: Joey <[email protected]> --------- Co-authored-by: Joey <[email protected]>
1 parent 20cfa8f commit e3050fd

10 files changed

+173
-34
lines changed

‎markets/bfp-market/contracts/modules/MarginModule.sol

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,7 @@ contract MarginModule is IMarginModule {
387387
runtime.lengthAfter = collateralAddresses.length;
388388
runtime.maxApproveAmount = type(uint256).max;
389389
runtime.previousSupportedCollaterals = globalMarginConfig.supportedCollaterals;
390+
390391
// Number of synth collaterals to configure exceeds system maxmium.
391392
if (runtime.lengthAfter > MAX_SUPPORTED_MARGIN_COLLATERALS) {
392393
revert ErrorUtil.MaxCollateralExceeded(
@@ -419,11 +420,19 @@ contract MarginModule is IMarginModule {
419420
delete globalMarginConfig.supportedCollaterals;
420421

421422
// Update with passed in configuration.
422-
address[] memory newSupportedCollaterals = new address[](runtime.lengthAfter);
423423
for (uint256 i = 0; i < runtime.lengthAfter; ) {
424424
address collateralAddress = collateralAddresses[i];
425+
426+
// Verify this is not a duplicate collateralAddress.
427+
for (uint256 j = i + 1; j < runtime.lengthAfter; j++) {
428+
if (collateralAddress == collateralAddresses[j]) {
429+
revert ErrorUtil.DuplicateEntries();
430+
}
431+
}
432+
425433
// Perform approve _once_ when this collateral is added as a supported collateral.
426434
ITokenModule(collateralAddress).approve(SYNTHETIX_CORE, runtime.maxApproveAmount);
435+
427436
// sUSD must have a 0x0 reward distributor.
428437
address distributor = rewardDistributors[i];
429438

@@ -451,13 +460,12 @@ contract MarginModule is IMarginModule {
451460
rewardDistributors[i],
452461
true
453462
);
454-
newSupportedCollaterals[i] = collateralAddress;
455463

456464
unchecked {
457465
++i;
458466
}
459467
}
460-
globalMarginConfig.supportedCollaterals = newSupportedCollaterals;
468+
globalMarginConfig.supportedCollaterals = collateralAddresses;
461469

462470
for (uint256 i = 0; i < runtime.lengthBefore; ) {
463471
address collateral = runtime.previousSupportedCollaterals[i];

‎markets/bfp-market/contracts/modules/PerpAccountModule.sol

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ contract PerpAccountModule is IPerpAccountModule {
221221
);
222222

223223
if (toId == fromId) {
224-
revert ErrorUtil.DuplicateAccountIds();
224+
revert ErrorUtil.DuplicateEntries();
225225
}
226226

227227
Runtime_splitAccount memory runtime;
@@ -442,7 +442,7 @@ contract PerpAccountModule is IPerpAccountModule {
442442

443443
// Cannot merge the same two accounts.
444444
if (toId == fromId) {
445-
revert ErrorUtil.DuplicateAccountIds();
445+
revert ErrorUtil.DuplicateEntries();
446446
}
447447

448448
Runtime_mergeAccounts memory runtime;

‎markets/bfp-market/contracts/modules/PerpRewardDistributorModule/PerpRewardDistributorFactoryModule.sol

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,15 @@ contract PerpRewardDistributorFactoryModule is IPerpRewardDistributorFactoryModu
6666
}
6767
}
6868

69+
// Inefficient but necessary O(n^2) loop to verify in-place there are no duplicates collateralTypes.
70+
for (uint256 i = 0; i < collateralTypesLength; i++) {
71+
for (uint256 j = i + 1; j < collateralTypesLength; j++) {
72+
if (data.collateralTypes[i] == data.collateralTypes[j]) {
73+
revert ErrorUtil.DuplicateEntries();
74+
}
75+
}
76+
}
77+
6978
// Create a new distributor by cloning an existing implementation.
7079
address distributorAddress = globalConfig.rewardDistributorImplementation.clone();
7180
IPerpRewardDistributor distributor = IPerpRewardDistributor(distributorAddress);

‎markets/bfp-market/contracts/storage/Position.sol

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -607,6 +607,11 @@ library Position {
607607
Margin.MarginValues memory marginValues,
608608
AddressRegistry.Data memory addresses
609609
) internal view returns (uint256) {
610+
// A zero sized position means there is no position.
611+
if (size == 0) {
612+
return type(uint256).max;
613+
}
614+
610615
// `margin / mm <= 1` means liquidation.
611616
(, uint256 mm) = getLiquidationMarginUsd(
612617
size,
@@ -615,6 +620,7 @@ library Position {
615620
marketConfig,
616621
addresses
617622
);
623+
618624
return marginValues.discountedMarginUsd.divDecimal(mm);
619625
}
620626

‎markets/bfp-market/contracts/utils/ErrorUtil.sol

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,8 +119,8 @@ library ErrorUtil {
119119
/// @notice Thrown when user trying to split an account with 0 porportion.
120120
error ZeroProportion();
121121

122-
/// @notice Thrown when duplicate account ids were found.
123-
error DuplicateAccountIds();
122+
/// @notice Thrown when duplicate entries in an array or otherwise was found and not expected.
123+
error DuplicateEntries();
124124

125125
/// @notice Thrown when user trying to merge accounts with positions on opposite sides.
126126
error InvalidPositionSide();

‎markets/bfp-market/test/integration/modules/LiquidationModule.getHealthFactor.test.ts

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
import assertBn from '@synthetixio/core-utils/utils/assertions/assert-bignumber';
2+
import { wei } from '@synthetixio/wei';
3+
import { bootstrap } from '../../bootstrap';
4+
import { bn, genBootstrap, genOneOf, genOrder, genSide, genTrader } from '../../generators';
5+
import {
6+
depositMargin,
7+
commitAndSettle,
8+
setMarketConfigurationById,
9+
setBaseFeePerGas,
10+
} from '../../helpers';
11+
import { BigNumber } from 'ethers';
12+
13+
describe('LiquidationModule', () => {
14+
const bs = bootstrap(genBootstrap());
15+
const { systems, provider, restore } = bs;
16+
17+
beforeEach(restore);
18+
19+
afterEach(async () => await setBaseFeePerGas(1, provider()));
20+
21+
describe('getHealthFactor', () => {
22+
it('should return expected healthFactor', async () => {
23+
const { BfpMarketProxy } = systems();
24+
25+
// Commit, settle, place position into liquidation, flag for liquidation. Additionally, we set
26+
// `desiredMarginUsdDepositAmount` to a low~ish value to prevent partial liquidations.
27+
const orderSide = genSide();
28+
const { trader, market, marketId, collateral, collateralDepositAmount } = await depositMargin(
29+
bs,
30+
genTrader(bs, { desiredMarginUsdDepositAmount: genOneOf([2000, 3000, 5000]) })
31+
);
32+
const order = await genOrder(bs, market, collateral, collateralDepositAmount, {
33+
desiredLeverage: 10,
34+
desiredSide: orderSide,
35+
});
36+
await commitAndSettle(bs, marketId, trader, order);
37+
38+
// Set a large enough liqCap to ensure a full liquidation.
39+
await setMarketConfigurationById(bs, marketId, { liquidationLimitScalar: bn(100) });
40+
41+
const healthFactor1 = await BfpMarketProxy.getHealthFactor(trader.accountId, marketId);
42+
assertBn.gt(healthFactor1, wei(1).toBN());
43+
44+
// Rekt.
45+
const newMarketOraclePrice = wei(order.oraclePrice)
46+
.mul(orderSide === 1 ? 0.89 : 1.11)
47+
.toBN();
48+
await market.aggregator().mockSetCurrentPrice(newMarketOraclePrice);
49+
50+
const healthFactor2 = await BfpMarketProxy.getHealthFactor(trader.accountId, marketId);
51+
assertBn.lt(healthFactor2, wei(1).toBN());
52+
});
53+
54+
it('should return max uint256 when no position found', async () => {
55+
const { BfpMarketProxy } = systems();
56+
57+
const { trader, marketId } = await depositMargin(bs, genTrader(bs));
58+
const healthFactor = await BfpMarketProxy.getHealthFactor(trader.accountId, marketId);
59+
60+
const maxUint256 = BigNumber.from(2).pow(256).sub(1);
61+
assertBn.equal(healthFactor, maxUint256);
62+
});
63+
});
64+
});

‎markets/bfp-market/test/integration/modules/MarginModule.test.ts

Lines changed: 59 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1853,30 +1853,7 @@ describe('MarginModule', async () => {
18531853
});
18541854
});
18551855

1856-
describe('setMarginMarginCollateralConfiguration', () => {
1857-
it('should revert when config arrays has mismatched lengths', async () => {
1858-
const { BfpMarketProxy } = systems();
1859-
const from = owner();
1860-
1861-
const collateralAddresses = [collaterals()[0].address(), collaterals()[1].address()];
1862-
const maxAllowables = genListOf(genNumber(3, 10), () => bn(genNumber(10_000, 100_000)));
1863-
const skewScales = genListOf(genNumber(3, 10), () => bn(genNumber(10_000, 100_000)));
1864-
const oracleNodeIds = genListOf(genNumber(3, 10), () => genBytes32());
1865-
const rewardDistributors = genListOf(genNumber(3, 10), () => genAddress());
1866-
1867-
await assertRevert(
1868-
BfpMarketProxy.connect(from).setMarginCollateralConfiguration(
1869-
collateralAddresses,
1870-
oracleNodeIds,
1871-
maxAllowables,
1872-
skewScales,
1873-
rewardDistributors
1874-
),
1875-
`ArrayLengthMismatch()`,
1876-
BfpMarketProxy
1877-
);
1878-
});
1879-
1856+
describe('setMarginCollateralConfiguration', () => {
18801857
it('should configure and return many collaterals configured', async () => {
18811858
const { BfpMarketProxy } = systems();
18821859
const from = owner();
@@ -2099,6 +2076,29 @@ describe('MarginModule', async () => {
20992076
assert.equal(collaterals.length, 0);
21002077
});
21012078

2079+
it('should revert when config arrays has mismatched lengths', async () => {
2080+
const { BfpMarketProxy } = systems();
2081+
const from = owner();
2082+
2083+
const collateralAddresses = [collaterals()[0].address(), collaterals()[1].address()];
2084+
const maxAllowables = genListOf(genNumber(3, 10), () => bn(genNumber(10_000, 100_000)));
2085+
const skewScales = genListOf(genNumber(3, 10), () => bn(genNumber(10_000, 100_000)));
2086+
const oracleNodeIds = genListOf(genNumber(3, 10), () => genBytes32());
2087+
const rewardDistributors = genListOf(genNumber(3, 10), () => genAddress());
2088+
2089+
await assertRevert(
2090+
BfpMarketProxy.connect(from).setMarginCollateralConfiguration(
2091+
collateralAddresses,
2092+
oracleNodeIds,
2093+
maxAllowables,
2094+
skewScales,
2095+
rewardDistributors
2096+
),
2097+
`ArrayLengthMismatch()`,
2098+
BfpMarketProxy
2099+
);
2100+
});
2101+
21022102
it('should revert when non-owner', async () => {
21032103
const { BfpMarketProxy } = systems();
21042104
const from = await traders()[0].signer.getAddress();
@@ -2125,7 +2125,7 @@ describe('MarginModule', async () => {
21252125
);
21262126
});
21272127

2128-
it('should revert when an collateralAddress is supplied as collateral', async () => {
2128+
it('should revert when a non collateralAddress is supplied as collateral', async () => {
21292129
const { BfpMarketProxy } = systems();
21302130
const from = owner();
21312131

@@ -2233,6 +2233,40 @@ describe('MarginModule', async () => {
22332233
);
22342234
});
22352235

2236+
it('should revert when a duplicate collateralAddress is found', async () => {
2237+
const { BfpMarketProxy } = systems();
2238+
const from = owner();
2239+
2240+
const collateralsToConfigure = [
2241+
collaterals()[0],
2242+
collaterals()[1],
2243+
collaterals()[0], // [0] is duplicated.
2244+
];
2245+
const collateralAddresses = collateralsToConfigure.map((c) => c.address());
2246+
const oracleNodeIds = genListOf(collateralsToConfigure.length, () => genBytes32());
2247+
const maxAllowables = genListOf(collateralsToConfigure.length, () =>
2248+
bn(genNumber(10_000, 100_000))
2249+
);
2250+
const skewScales = genListOf(collateralsToConfigure.length, () =>
2251+
bn(genNumber(10_000, 100_000))
2252+
);
2253+
const rewardDistributors = collateralsToConfigure.map(({ rewardDistributorAddress }) =>
2254+
rewardDistributorAddress()
2255+
);
2256+
2257+
await assertRevert(
2258+
BfpMarketProxy.connect(from).setMarginCollateralConfiguration(
2259+
collateralAddresses,
2260+
oracleNodeIds,
2261+
maxAllowables,
2262+
skewScales,
2263+
rewardDistributors
2264+
),
2265+
`DuplicateEntries()`,
2266+
BfpMarketProxy
2267+
);
2268+
});
2269+
22362270
it('should revoke/approve collateral with 0/maxUint');
22372271
});
22382272

‎markets/bfp-market/test/integration/modules/PerpAccountModule.mergeAccounts.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ describe('PerpAccountModule mergeAccounts', () => {
102102
fromTrader.accountId,
103103
1
104104
),
105-
`DuplicateAccountIds`,
105+
`DuplicateEntries`,
106106
BfpMarketProxy
107107
);
108108
});

‎markets/bfp-market/test/integration/modules/PerpAccountModule.splitAccount.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ describe('PerpAccountModule splitAccount', () => {
5050
fromTrader.accountId,
5151
1
5252
),
53-
`DuplicateAccountIds`,
53+
`DuplicateEntries`,
5454
BfpMarketProxy
5555
);
5656
});

‎markets/bfp-market/test/integration/modules/PerpRewardDistributorFactoryModule.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,24 @@ describe('PerpRewardDistributorFactoryModule', () => {
174174
BfpMarketProxy
175175
);
176176
});
177+
178+
it('should revert when there are duplicate poolCollaterals', async () => {
179+
const { BfpMarketProxy } = systems();
180+
181+
const from = owner();
182+
const collateralType = genAddress();
183+
184+
await assertRevert(
185+
BfpMarketProxy.connect(from).createRewardDistributor({
186+
poolId: pool().id,
187+
collateralTypes: [collateralType, genAddress(), collateralType],
188+
name: genBytes32(),
189+
token: genAddress(),
190+
}),
191+
'DuplicateEntries()',
192+
BfpMarketProxy
193+
);
194+
});
177195
});
178196

179197
describe('Core.RewardsManagerModule.claimReward', () => {

0 commit comments

Comments
 (0)