sherlock-audit / 2023-09-perennial-judging

0 stars 0 forks source link

0xVinylDavyl - bitwise left shift operations on slot0, slot1, and slot2 without explicit checks for arithmetic overflow. If the values in these slots exceed the expected bit length, it leads to unintended behavior #24

Closed sherlock-admin closed 11 months ago

sherlock-admin commented 11 months ago

0xVinylDavyl

medium

bitwise left shift operations on slot0, slot1, and slot2 without explicit checks for arithmetic overflow. If the values in these slots exceed the expected bit length, it leads to unintended behavior

Summary

The code uses bitwise left shift operations on slot0, slot1, and slot2 without explicit checks for arithmetic overflow. If the values in these slots exceed the expected bit length, it can lead to unintended behavior.

Vulnerability Detail

bitwise left shift operations are used to extract values from slot0, slot1, and slot2. However, these operations do not include explicit checks to prevent arithmetic overflow. If the values stored in these slots exceed the expected bit length for the extracted values, it may result in unintended behavior, potentially impacting the protocol's functionality.

Impact

Arithmetic overflow in bitwise left shift operations can lead to incorrect parameter values being stored or read from slot0, slot1, and slot2. Such incorrect values can have a cascading effect, affecting various aspects of the protocol's behavior, which may lead to unexpected and undesirable outcomes.

Code Snippet

This vulnerability is a design issue and can be demonstrated by setting values in slot0, slot1, and slot2 that exceed the bit length expectations. For example, if values beyond the 256-bit limit are stored in these slots, the left shift operations can behave unpredictably.

Here is an example of how the vulnerability can be demonstrated:

// Set slot0 to a value that exceeds the bit length
riskParameterStorage.slot0 = 2**300;

// Attempt to extract a value from slot0
uint256 extractedValue = (riskParameterStorage.slot0 << (256 - 24)) >> (256 - 24);

In this example, riskParameterStorage.slot0 is set to a value that significantly exceeds the expected bit length. The subsequent left shift operation can produce unpredictable results.

https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial/contracts/types/RiskParameter.sol#L174

Tool used

Manual Review

Recommendation

The risk of arithmetic overflow in bitwise left shift operations, it is recommended to implement checks to ensure that the values stored in slot0, slot1, and slot2 do not exceed the expected bit length for the extracted values. Additionally, you can restrict the values to within the acceptable range. Below is an example of how this can be done:

// Define the maximum allowed bit length for extracted values
uint256 maxBitLength = 256;  // Adjust as per your specific requirements

// Ensure that the value in slot0 does not exceed the allowed bit length
if (riskParameterStorage.slot0 >= 2**maxBitLength) {
    // Handle the error: Value in slot0 exceeds the allowed bit length
    revert("Value in slot0 exceeds the allowed bit length");
}

// Ensure that the value in slot1 does not exceed the allowed bit length
if (riskParameterStorage.slot1 >= 2**maxBitLength) {
    // Handle the error: Value in slot1 exceeds the allowed bit length
    revert("Value in slot1 exceeds the allowed bit length");
}

// Ensure that the value in slot2 does not exceed the allowed bit length
if (riskParameterStorage.slot2 >= 2**maxBitLength) {
    // Handle the error: Value in slot2 exceeds the allowed bit length
    revert("Value in slot2 exceeds the allowed bit length");
}

// Proceed with the bitwise left shift operations knowing that values are within the allowed range
uint256 extractedValue0 = (riskParameterStorage.slot0 << (maxBitLength - 24)) >> (maxBitLength - 24);
uint256 extractedValue1 = (riskParameterStorage.slot1 << (maxBitLength - 48)) >> (maxBitLength - 48);
uint256 extractedValue2 = (riskParameterStorage.slot2 << (maxBitLength - 64)) >> (maxBitLength - 64);

In this recommendation, we first define a maximum allowed bit length (maxBitLength) for extracted values. Then, we check that the values in slot0, slot1, and slot2 do not exceed this maximum allowed bit length. If any of them do, an error is raised, and the operation is halted to prevent arithmetic overflow. This approach ensures that values are within a safe range before performing left shift operations. Adjust the maxBitLength value to match your specific requirements.

sherlock-admin commented 11 months ago

4 comment(s) were left on this issue during the judging contest.

panprog commented:

invalid because the code is correct, advice is generic

n33k commented:

invalid, ai generated groundless report

darkart commented:

The same as 14

polarzero commented:

Invalid. There is no evidence that this would indeed put the funds at risk or cause any malfunction in the protocol.