Summary
The function GlobalConfiguration::getAccountsWithActivePositions allows for out-of-bounds access when creating the accountsIds array. Specifically, the line accountsIds = new uint128[](upperBound - lowerBound + 1); assumes that the range between lowerBound and upperBound is valid and within the limits of the accountsIdsWithActivePositions array.
This assumption can lead to the creation of an array with invalid indices, resulting in potential out-of-bounds access errors during runtime.
Vulnerability Details
If upperBound is set to a value greater than the length of globalConfiguration.accountsIdsWithActivePositions, it will result in an out-of-bounds access when trying to populate the accountsIds array.
function getAccountsWithActivePositions(
uint256 lowerBound,
uint256 upperBound
)
external
view
returns (uint128[] memory accountsIds)
{
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
@> accountsIds = new uint128[](upperBound - lowerBound + 1);
uint256 index = 0;
for (uint256 i = lowerBound; i <= upperBound; i++) {
@> accountsIds[index] = uint128(globalConfiguration.accountsIdsWithActivePositions.at(i));
index++;
}
}
Proof of Code
Modify the test GetAccountsWithActivePositions_Integration_Test::testFuzz_GivenThereAreAccountsWithActivePositions and bring the following modifications:
function testFuzz_GivenThereAreAccountsWithActivePositions(
uint256 initialMarginRate,
uint256 marginValueUsd,
bool isLong,
uint256 marketId,
+ uint256 upperBound,
+ uint256 lowerBound
)
external
{
changePrank({ msgSender: users.naruto.account });
// ...
changePrank({ msgSender: marketOrderKeeper });
perpsEngine.fillMarketOrder(tradingAccountId, fuzzMarketConfig.marketId, mockSignedReport);
- uint128[] memory accountsIds = perpsEngine.getAccountsWithActivePositions(0, 0);
+ uint128[] memory accountsIds = perpsEngine.getAccountsWithActivePositions(lowerBound, upperBound);
assertEq(accountsIds[0], tradingAccountId, "getAccountsWithActivePositions: invalid account id");
}
And run the test with forge test --mt testFuzz_GivenThereAreAccountsWithActivePositions . You’ll see the vulnerability::
2024-07-zaros % forge test --mt testFuzz_GivenThereAreAccountsWithActivePositions
[⠆] Compiling...
//...
Failing tests:
Encountered 1 failing test in test/integration/perpetuals/global-configuration-branch/getAccountsWithActivePositions/getAccountsWithActivePositions.t.sol:GetAccountsWithActivePositions_Integration_Test
[FAIL. Reason: panic: array out-of-bounds access (0x32); counterexample: calldata=0xe66b0b11000000000000000000000000000000000000000000000000000000000000269f00000000000000000000000000000000000000000000000000000000000003c300000000000000000000000000000000000000000000000000000000000000010d2a6872ef858a7f8ead18dc4f3f2e8d35c853d47e2816cbb9cdd49202554e1700000000000000000000000000000000000000000000000000000000000013110000000000000000000000000000000000000000000000000000000000000e90 args=[9887, 963, true, 5954995488581714757959077518955326425251219611844355894082561262061582896663 [5.954e75], 4881, 3728]] testFuzz_GivenThereAreAccountsWithActivePositions(uint256,uint256,bool,uint256,uint256,uint256) (runs: 0, μ: 0, ~: 0)
Encountered a total of 1 failing tests, 0 tests succeeded
Impact
Given that this function is intended for use by external integrators such as the UI and backend, the potential impacts include:
Denial of Service (DoS): The function could cause integrators services to crash or behave unpredictably if an out-of-bounds access occurs.
Data Integrity Issues: Accessing invalid memory could result in incorrect data being returned, leading to potential misinformation or faulty operations based on incorrect data.
Tools Used
Manual Review & Fuzz Testing
Recommendations
Implement bounds checking to ensure that the lowerBound and upperBound values are within the valid range of the accountsIdsWithActivePositions array:
function getAccountsWithActivePositions(
uint256 lowerBound,
uint256 upperBound
)
external
view
returns (uint128[] memory accountsIds)
{
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
+ uint256 activePositionsCount = globalConfiguration.accountsIdsWithActivePositions.length();
+ if (lowerBound > upperBound || upperBound >= activePositionsCount) {
+ revert Errors.InvalidBounds();
+ }
accountsIds = new uint128[](upperBound - lowerBound + 1);
uint256 index = 0;
for (uint256 i = lowerBound; i <= upperBound; i++) {
accountsIds[index] = uint128(globalConfiguration.accountsIdsWithActivePositions.at(i));
index++;
}
}
