code-423n4 / 2023-11-panoptic-findings

0 stars 0 forks source link

Incorrect Handling of Negative values in toRightSlot() and toLeftSlot() #618

Closed c4-bot-6 closed 11 months ago

c4-bot-6 commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/types/LeftRight.sol#L108 https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/types/LeftRight.sol#L75

Vulnerability details

Impact

The vulnerability arises when attempting to store a negative value in the right slot using the toRightSlot functions. The code incorrectly adds int256(right) directly to the original value, potentially causing an arithmetic overflow. The recommended correction involves using the bitwise AND operation with the RIGHT_HALF_BIT_MASK constant to prevent overflow and ensure proper handling of negative values.

Impacts on the project are

  1. Arithmetic Overflow: The incorrect handling of negative values in arithmetic operations may lead to arithmetic overflow. This occurs when the result of an addition operation exceeds the maximum representable value for the data type (int256 in this case). Arithmetic overflow can result in unexpected and incorrect values, potentially compromising the integrity of calculations.

  2. Data Corruption: Mishandling negative values can corrupt the data stored in the smart contract. If the arithmetic overflow occurs during a data update or calculation, the stored values may become inconsistent or incorrect, leading to data corruption.

Proof of Concept

// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity ^0.8.0;

library LeftRight {
    int256 internal constant RIGHT_HALF_BIT_MASK = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;

    // Vulnerable to arithmetic overflow for negative values
    function vulnerableOperation(int256 x, int128 right) internal pure returns (int256) {
        unchecked {
            // Incorrect: Direct addition without proper handling
            return x + int256(right);
        }
    }
}

contract DemoContract {
    using LeftRight for int256;

    // Function to demonstrate the vulnerability
    function triggerVulnerability(int256 x, int128 right) external pure returns (int256) {
        // Triggering the vulnerable operation
        return x.vulnerableOperation(right);
    }
}

contract DemoAttack {
    // Function to demonstrate the vulnerability
    function attackDemoContract() external pure {
        // Create an instance of DemoContract
        DemoContract demoContract = new DemoContract();

        // Trigger the vulnerability with a negative value
        demoContract.triggerVulnerability(type(int256).min, int128(-1));
    }
}

here the DemoAttack uses an instance of DemoContract and calls the triggerVulnerability function with a negative value (int128(-1)). When executed, this should showcase the vulnerability and potentially lead to an arithmetic overflow.

Tools Used

  1. VSCode
  2. Slinther

Recommended Mitigation Steps

Update toRightSlot for int256 Values:

  1. Modify the toRightSlot function to use the bitwise AND operation with the RIGHT_HALF_BIT_MASK constant when dealing with negative values.

function toRightSlot(int256 self, int128 right) internal pure returns (int256) { unchecked { // Use bitwise AND operation with RIGHT_HALF_BIT_MASK to prevent overflow return self + (int256(right) & RIGHT_HALF_BIT_MASK); } }


2. Update ```toLeftSlot``` for int256 Values (if necessary):

```solidity
function toLeftSlot(int256 self, int128 left) internal pure returns (int256) {
    unchecked {
        // Use bitwise AND operation with RIGHT_HALF_BIT_MASK to prevent overflow
        return self + (int256(left) << 128);
    }
}```

## Assessed type

Under/Overflow
c4-judge commented 11 months ago

Picodes marked the issue as unsatisfactory: Invalid