Skip to content

Commit 7f949d1

Browse files
KogaroshiKogaroshi
authored
fix: prevent key collision on KeyValueList add() (#901)
1 parent 427cd16 commit 7f949d1

File tree

5 files changed

+180
-33
lines changed

5 files changed

+180
-33
lines changed

‎snapshots/Spoke.Getters.json‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"getUserAccountData: supplies: 0, borrows: 0": "12164",
3-
"getUserAccountData: supplies: 1, borrows: 0": "47260",
4-
"getUserAccountData: supplies: 2, borrows: 0": "77471",
5-
"getUserAccountData: supplies: 2, borrows: 1": "98212",
6-
"getUserAccountData: supplies: 2, borrows: 2": "117607"
3+
"getUserAccountData: supplies: 1, borrows: 0": "47254",
4+
"getUserAccountData: supplies: 2, borrows: 0": "77459",
5+
"getUserAccountData: supplies: 2, borrows: 1": "98200",
6+
"getUserAccountData: supplies: 2, borrows: 2": "117595"
77
}

‎snapshots/Spoke.Operations.ZeroRiskPremium.json‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
{
2-
"borrow: first": "199038",
3-
"borrow: second action, same reserve": "179559",
4-
"liquidationCall: full": "291027",
5-
"liquidationCall: partial": "315121",
6-
"permitReserve + repay (multicall)": "235806",
2+
"borrow: first": "199032",
3+
"borrow: second action, same reserve": "179553",
4+
"liquidationCall: full": "291015",
5+
"liquidationCall: partial": "315109",
6+
"permitReserve + repay (multicall)": "235800",
77
"permitReserve + supply (multicall)": "141050",
88
"permitReserve + supply + enable collateral (multicall)": "176287",
9-
"repay: full": "151770",
10-
"repay: partial": "176233",
9+
"repay: full": "151764",
10+
"repay: partial": "176227",
1111
"setUserPositionManagerWithSig: disable": "44918",
1212
"setUserPositionManagerWithSig: enable": "68947",
1313
"supply + enable collateral (multicall)": "154229",
@@ -17,16 +17,16 @@
1717
"supply: second action, same reserve": "103081",
1818
"updateUserDynamicConfig: 1 collateral": "73761",
1919
"updateUserDynamicConfig: 2 collaterals": "88621",
20-
"updateUserRiskPremium: 1 borrow": "95282",
21-
"updateUserRiskPremium: 2 borrows": "111374",
20+
"updateUserRiskPremium: 1 borrow": "95276",
21+
"updateUserRiskPremium: 2 borrows": "111368",
2222
"usingAsCollateral: 0 borrows, enable": "58976",
23-
"usingAsCollateral: 1 borrow, disable": "105529",
23+
"usingAsCollateral: 1 borrow, disable": "105523",
2424
"usingAsCollateral: 1 borrow, enable": "32298",
25-
"usingAsCollateral: 2 borrows, disable": "127994",
25+
"usingAsCollateral: 2 borrows, disable": "127988",
2626
"usingAsCollateral: 2 borrows, enable": "41876",
2727
"withdraw: 0 borrows, full": "127742",
28-
"withdraw: 0 borrows, partial": "132789",
29-
"withdraw: 1 borrow, partial": "161771",
30-
"withdraw: 2 borrows, partial": "184224",
28+
"withdraw: 0 borrows, partial": "132783",
29+
"withdraw: 1 borrow, partial": "161765",
30+
"withdraw: 2 borrows, partial": "184218",
3131
"withdraw: non collateral": "126851"
3232
}

‎snapshots/Spoke.Operations.json‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
{
2-
"borrow: first": "269207",
3-
"borrow: second action, same reserve": "212728",
4-
"liquidationCall: full": "324413",
5-
"liquidationCall: partial": "348507",
6-
"permitReserve + repay (multicall)": "269185",
2+
"borrow: first": "269201",
3+
"borrow: second action, same reserve": "212722",
4+
"liquidationCall: full": "324401",
5+
"liquidationCall: partial": "348495",
6+
"permitReserve + repay (multicall)": "269179",
77
"permitReserve + supply (multicall)": "141050",
88
"permitReserve + supply + enable collateral (multicall)": "176287",
9-
"repay: full": "146411",
10-
"repay: partial": "209612",
9+
"repay: full": "146405",
10+
"repay: partial": "209606",
1111
"setUserPositionManagerWithSig: disable": "44918",
1212
"setUserPositionManagerWithSig: enable": "68947",
1313
"supply + enable collateral (multicall)": "154229",
@@ -17,16 +17,16 @@
1717
"supply: second action, same reserve": "103081",
1818
"updateUserDynamicConfig: 1 collateral": "73761",
1919
"updateUserDynamicConfig: 2 collaterals": "88621",
20-
"updateUserRiskPremium: 1 borrow": "177369",
21-
"updateUserRiskPremium: 2 borrows": "262734",
20+
"updateUserRiskPremium: 1 borrow": "177363",
21+
"updateUserRiskPremium: 2 borrows": "262728",
2222
"usingAsCollateral: 0 borrows, enable": "58976",
23-
"usingAsCollateral: 1 borrow, disable": "187616",
23+
"usingAsCollateral: 1 borrow, disable": "187610",
2424
"usingAsCollateral: 1 borrow, enable": "32298",
25-
"usingAsCollateral: 2 borrows, disable": "287353",
25+
"usingAsCollateral: 2 borrows, disable": "287347",
2626
"usingAsCollateral: 2 borrows, enable": "41876",
2727
"withdraw: 0 borrows, full": "127742",
28-
"withdraw: 0 borrows, partial": "132789",
29-
"withdraw: 1 borrow, partial": "241358",
30-
"withdraw: 2 borrows, partial": "341084",
28+
"withdraw: 0 borrows, partial": "132783",
29+
"withdraw: 1 borrow, partial": "241352",
30+
"withdraw: 2 borrows, partial": "341078",
3131
"withdraw: non collateral": "126851"
3232
}

‎src/spoke/libraries/KeyValueList.sol‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,9 @@ library KeyValueList {
3535
}
3636

3737
/// @notice Inserts packed `key`, `value` at `idx`. Reverts if data exceeds maximum allowed size.
38+
/// @dev Reverts if `key` equals or exceeds the `_MAX_KEY` value and reverts if `value` equals or exceeds the `_MAX_VALUE` value.
3839
function add(List memory self, uint256 idx, uint256 key, uint256 value) internal pure {
39-
require(key <= _MAX_KEY && value <= _MAX_VALUE, MaxDataSizeExceeded());
40+
require(key < _MAX_KEY && value < _MAX_VALUE, MaxDataSizeExceeded());
4041
self._inner[idx] = pack(key, value);
4142
}
4243

‎tests/unit/KeyValueList.t.sol‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,123 @@ pragma solidity ^0.8.0;
55
import {Test} from 'forge-std/Test.sol';
66
import {KeyValueList} from 'src/spoke/libraries/KeyValueList.sol';
77

8+
/// forge-config: default.allow_internal_expect_revert = true
89
contract KeyValueListTest is Test {
910
using KeyValueList for KeyValueList.List;
1011

12+
function test_add_unique() public {
13+
/// @dev needed for reverts not to block the test
14+
KeyValueListWrapper wrapper = new KeyValueListWrapper();
15+
KeyValueList.List memory list = KeyValueList.init(11);
16+
17+
list = wrapper.add(list, 0, 1, 1);
18+
list = wrapper.add(list, 1, 100, 1e15);
19+
list = wrapper.add(list, 2, 100, 5e20);
20+
list = wrapper.add(list, 3, 5e4, 5e20);
21+
list = wrapper.add(list, 4, 5e4, 1e12);
22+
list = wrapper.add(list, 5, 45e6, 2.5e50);
23+
list = wrapper.add(list, 6, 45e6, 10);
24+
25+
list = wrapper.add(list, 7, type(uint32).max - 1, 10000000000);
26+
27+
vm.expectRevert(KeyValueList.MaxDataSizeExceeded.selector);
28+
wrapper.add(list, 8, type(uint32).max, 10000000000);
29+
30+
vm.expectRevert(KeyValueList.MaxDataSizeExceeded.selector);
31+
wrapper.add(list, 8, 45e8, 10000000000);
32+
33+
vm.expectRevert(KeyValueList.MaxDataSizeExceeded.selector);
34+
wrapper.add(list, 8, 75e9, 10000000000);
35+
36+
vm.expectRevert(KeyValueList.MaxDataSizeExceeded.selector);
37+
wrapper.add(list, 9, 5e6, 2.696e67);
38+
39+
vm.expectRevert(KeyValueList.MaxDataSizeExceeded.selector);
40+
wrapper.add(list, 9, 5e6, 5e70);
41+
42+
vm.expectRevert(KeyValueList.MaxDataSizeExceeded.selector);
43+
wrapper.add(list, 9, 5e6, 12.5e75);
44+
45+
vm.expectRevert(KeyValueList.MaxDataSizeExceeded.selector);
46+
wrapper.add(list, 9, 5e6, type(uint224).max);
47+
48+
list = wrapper.add(list, 10, 5e6, type(uint224).max - 1);
49+
50+
uint256 returnedKey;
51+
uint256 returnedValue;
52+
(returnedKey, returnedValue) = list.get(0);
53+
assertEq(returnedKey, 1);
54+
assertEq(returnedValue, 1);
55+
(returnedKey, returnedValue) = list.get(1);
56+
assertEq(returnedKey, 100);
57+
assertEq(returnedValue, 1e15);
58+
(returnedKey, returnedValue) = list.get(2);
59+
assertEq(returnedKey, 100);
60+
assertEq(returnedValue, 5e20);
61+
(returnedKey, returnedValue) = list.get(3);
62+
assertEq(returnedKey, 5e4);
63+
assertEq(returnedValue, 5e20);
64+
(returnedKey, returnedValue) = list.get(4);
65+
assertEq(returnedKey, 5e4);
66+
assertEq(returnedValue, 1e12);
67+
(returnedKey, returnedValue) = list.get(5);
68+
assertEq(returnedKey, 45e6);
69+
assertEq(returnedValue, 2.5e50);
70+
(returnedKey, returnedValue) = list.get(6);
71+
assertEq(returnedKey, 45e6);
72+
assertEq(returnedValue, 10);
73+
(returnedKey, returnedValue) = list.get(7);
74+
assertEq(returnedKey, type(uint32).max - 1);
75+
assertEq(returnedValue, 10000000000);
76+
(returnedKey, returnedValue) = list.get(8);
77+
assertEq(returnedKey, 0);
78+
assertEq(returnedValue, 0);
79+
(returnedKey, returnedValue) = list.get(9);
80+
assertEq(returnedKey, 0);
81+
assertEq(returnedValue, 0);
82+
(returnedKey, returnedValue) = list.get(10);
83+
assertEq(returnedKey, 5e6);
84+
assertEq(returnedValue, type(uint224).max - 1);
85+
}
86+
87+
function test_fuzz_add(uint256 key, uint256 value) public {
88+
/// @dev needed for reverts not to block the test
89+
KeyValueListWrapper wrapper = new KeyValueListWrapper();
90+
KeyValueList.List memory list = KeyValueList.init(5);
91+
92+
if (key >= KeyValueList._MAX_KEY || value >= KeyValueList._MAX_VALUE) {
93+
vm.expectRevert(KeyValueList.MaxDataSizeExceeded.selector);
94+
wrapper.add(list, 0, key, value);
95+
} else {
96+
list.add(0, key, value);
97+
}
98+
99+
if (key < KeyValueList._MAX_KEY && value < KeyValueList._MAX_VALUE) {
100+
(uint256 storedKey, uint256 storedValue) = list.get(0);
101+
assertEq(storedKey, key);
102+
assertEq(storedValue, value);
103+
}
104+
}
105+
106+
function test_fuzz_add_unique(uint256 seed, uint256 size) public pure {
107+
size = bound(size, 1, 1e2);
108+
uint256[] memory keys = _generateRandomUint256Array(size, seed, 1, type(uint32).max - 1);
109+
uint256[] memory values = _generateRandomUint256Array(size, seed, 1, type(uint224).max - 1);
110+
KeyValueList.List memory list = KeyValueList.init(keys.length);
111+
for (uint256 i; i < keys.length; ++i) {
112+
list.add(i, keys[i], values[i]);
113+
}
114+
for (uint256 i; i < keys.length; ++i) {
115+
(uint256 key, uint256 value) = list.get(i);
116+
assertEq(key, keys[i]);
117+
assertEq(value, values[i]);
118+
// No key should return as 0 unless the set key is 0 or the cell is not initialized
119+
assertGt(key, 0);
120+
// No value should return as 0 unless the set value is 0 or the cell is not initialized
121+
assertGt(value, 0);
122+
}
123+
}
124+
11125
function test_fuzz_sortByKey(uint256[] memory seed) public pure {
12126
vm.assume(seed.length > 0);
13127
KeyValueList.List memory list = KeyValueList.init(seed.length);
@@ -118,4 +232,36 @@ contract KeyValueListTest is Test {
118232
function _truncateValue(uint256 value) internal pure returns (uint256) {
119233
return value % KeyValueList._MAX_VALUE;
120234
}
235+
236+
function _generateRandomUint256Array(
237+
uint256 size,
238+
uint256 seed,
239+
uint256 lowerBound,
240+
uint256 upperBound
241+
) internal pure returns (uint256[] memory) {
242+
seed = seed % 1e77;
243+
if (size == 0) return new uint256[](0);
244+
uint256[] memory result = new uint256[](size);
245+
for (uint256 i; i < size; ++i) {
246+
result[i] =
247+
(uint256((keccak256(abi.encode(seed + i)))) % (upperBound - lowerBound + 1)) +
248+
lowerBound;
249+
}
250+
return result;
251+
}
252+
}
253+
254+
contract KeyValueListWrapper {
255+
using KeyValueList for KeyValueList.List;
256+
257+
function add(
258+
KeyValueList.List memory list,
259+
uint256 idx,
260+
uint256 key,
261+
uint256 value
262+
) external pure returns (KeyValueList.List memory returnList) {
263+
returnList = list;
264+
returnList.add(idx, key, value);
265+
return returnList;
266+
}
121267
}

0 commit comments

Comments
 (0)