Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Insufficient loop index (uint8) in getUserDeposits: risk of infinite loop due to an Overflow

x0t0wt1w

Summary

Insufficient loop index (uint8) in getUserDeposits(): risk of infinite loop due to an Overflow

Vulnerability Details

function getUserDeposits(address user) external view returns (uint256[] memory depsitIds) {
uint256 length = EnumerableSet.length(userDeposits[user]);
depositIds = new uint256[](length);
for (uint8 i = 0; i < length; ) {
depositIds[i] = EnumerableSet.at(userDeposits[user], i);
unchecked {
i = i + 1; } } }

In the function, the index i of the loop is an uint8 and can therefore only have a maximum value of 255.
At the end of the loop, unchecked implies that the function will not revert if i = 256, but i will take the value 0 (Overflow).

If a user has more than 255 deposits, the variable i will never reach length (>255).
The loop will therefore run indefinitely until there is no more gas and will revert with the “out of gas” error.

Proof of Concept

In the test below, the getUserDeposits() function is called once after Alice has made 255 deposits.
On the second call, Alice has made a 256th deposit, and this time the function reverts as the loop in the function becomes infinite.

Add this function in PerpetualVault.t.sol :

function test_GetUserDeposits() external {
address alice = makeAddr("alice");
payable(alice).transfer(100 ether);
for (uint8 i = 0; i <= 254; i++) { //Alice makes 255 deposits
depositFixture(alice, 1e10);
}
uint256[] memory depositIds = PerpetualVault(vault).getUserDeposits(
alice
);
assertEq(depositIds.length, 255); // Checking that Alice has 255 deposits
depositFixture(alice, 1e10); // 256th deposit
vm.expectRevert(); // "Out of Gas" error
depositIds = PerpetualVault(vault).getUserDeposits(alice); // Infinite loop
}

Impact

The getUserDeposits() function will always revert with the “out of gas” error for users with more than 255 deposits. It is therefore impossible to retrieve all the deposits id of this user.

In the front-end, this list of deposits allows the user to choose which deposit to withdraw.

Users cannot withdraw their deposits via the protocol's front-end.

Impact is HIGH

Likelihood

The likelihood is LOW because 256 deposits with a minimum of $1000 is a lot, but over 1 or more years this number can be reached (via DCA for example)

Severity

MEDIUM

Tools Used

Manual review

Recommendations

Increase index size of the loop in getUserDeposits() to anticipate cases where a user has a large number of deposits.

Updates

Lead Judging Commences

n0kto Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.