From 08a0d28a728906990ca569fb1b42c576d654eb7a Mon Sep 17 00:00:00 2001 From: konfushon Date: Fri, 7 Jun 2024 19:10:14 +0300 Subject: [PATCH 1/7] Add memory-corruption-due-to-insufficient-allocation.md --- README.md | 1 + ...rruption-due-to-insufficient-allocation.md | 175 ++++++++++++++++++ 2 files changed, 176 insertions(+) create mode 100644 vulnerabilities/memory-corruption-due-to-insufficient-allocation.md diff --git a/README.md b/README.md index eea72fa..477667a 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,7 @@ - [Shadowing State Variables](./vulnerabilities/shadowing-state-variables.md) - [Weak Sources of Randomness from Chain Attributes](./vulnerabilities/weak-sources-randomness.md) - [Missing Protection against Signature Replay Attacks](./vulnerabilities/missing-protection-signature-replay.md) +- [Memory Corruption Due To Insufficient Memory Allocation](./vulnerabilities/memory-corruption-due-to-insufficient-allocation.md) - [Requirement Validation](./vulnerabilities/requirement-violation.md) - [Write to Arbitrary Storage Location](./vulnerabilities/arbitrary-storage-location.md) - [Hash Collision when using abi.encodePacked() with Multiple Variable-Length Arguments](./vulnerabilities/hash-collision.md) diff --git a/vulnerabilities/memory-corruption-due-to-insufficient-allocation.md b/vulnerabilities/memory-corruption-due-to-insufficient-allocation.md new file mode 100644 index 0000000..ed9acc7 --- /dev/null +++ b/vulnerabilities/memory-corruption-due-to-insufficient-allocation.md @@ -0,0 +1,175 @@ +# Memory Corruption Due to Insufficient Memory Allocation + +Memory corruption occurs when a program writes outside the bounds of allocated memory, potentially overwriting other important data. + +In Solidity, especially when using inline assembly, it's crucial to allocate enough memory to avoid memory corruption issues. + +Below is an example heavily borrowed off and simplified from [this](https://solodit.xyz/issues/memory-corruption-in-buffer-fixed-consensys-ens-permanent-registrar-markdown) Ethereum Name Service(ENS) audit to demonstrate the issue: + +```solidity +// SPDX-License-Identifier: MIT +pragma solidity 0.8.25; + +import {Test} from "forge-std/Test.sol"; + +// finding : https://solodit.xyz/issues/memory-corruption-in-buffer-fixed-consensys-ens-permanent-registrar-markdown +// code from : https://github.com/ensdomains/buffer/blob/c06d796e2f6086473d229a96c2eb75053a19b8ec/contracts/Buffer.sol + +library Buffer { + + struct buffer { + bytes buf; + uint capacity; + } + + function init(buffer memory buf, uint capacity) internal pure returns(buffer memory) { + if (capacity % 32 != 0) { + capacity += 32 - (capacity % 32); + } + // Allocate space for the buffer data + buf.capacity = capacity; + + assembly { + let ptr := mload(0x40) + mstore(buf, ptr) + mstore(ptr, 0) + + // does not account for length 0x20 (32), even though the `write` function does + mstore(0x40, add(ptr, capacity)) + /// @audit fix: mstore(0x40, add(32, add(ptr, capacity))) + } + return buf; + } + + function append(buffer memory buf, bytes memory data) internal pure returns (buffer memory) { + return write(buf, buf.buf.length, bytes32(data), data.length); + } + + function write(buffer memory buf, uint off, bytes32 data, uint len) private pure returns(buffer memory) { + if (len + off > buf.capacity) { + resize(buf, max(buf.capacity, len) * 2); + } + + uint mask = 256 ** len - 1; + // Right-align data + data = data >> (8 * (32 - len)); + + assembly { + // Memory address of the buffer data + let bufptr := mload(buf) + + // Address = buffer address + sizeof(buffer length) + off + len + let dest := add(add(bufptr, off), len) + + mstore(dest, or(and(mload(dest), not(mask)), data)) + // Update buffer length if we extended it + + if gt(add(off, len), mload(bufptr)) { + mstore(bufptr, add(off, len)) + } + } + return buf; + } + + function resize(buffer memory buf, uint capacity) private pure { + bytes memory oldbuf = buf.buf; + init(buf, capacity); + append(buf, oldbuf); + } + + function max(uint a, uint b) private pure returns(uint) { + if (a > b) { + return a; + } + return b; + } + +} +``` + +A library `Buffer` is used to manage a dynamically-sized byte array (`bytes`) with a specific capacity. The library contains functions to initialize the buffer (`init`), add data (`append`) and write data to the buffer (`write`) + + +```solidity +contract BufferTest is Test { + using Buffer for Buffer.buffer; + + function test_MemoryCorruption() external { + Buffer.buffer memory buffer; + buffer.init(1); + + // `foo` immediately follows buffer.buf in memory + bytes memory foo = new bytes(0x01); + + // sanity check passes + assert(1 == foo.length); + + // append "A" 0x41 (65) to buffer. This gets written to the high order byte of `foo.length` + buffer.append("A"); + + /** + * foo.length == 0x4100000000000000000000000000000000000000000000000000000000000001 + * == 29400335157912315244266070412362164103369332044010299463143527189509193072641 + * the below assertion passes showing the memory corruption + */ + + assertEq(29400335157912315244266070412362164103369332044010299463143527189509193072641, foo.length); + + } +} +``` + +- The above foundry test is borrowed from the amazing work of [Dacian](https://x.com/DevDacian) + +### Memory Corruption Explained + +1. **Initialization**: + - `Buffer::init` initializes a buffer with a specified capacity. + - Memory allocation is done using `mstore`, and the Free Memory Pointer Address (FMPA) is updated. + +2. **Memory Corruption**: + - In the test, `foo` is a new byte array created right after the buffer in memory. + - When `buffer.append("A")` is called, it writes data into the buffer. + - Due to insufficient memory allocation, this write operation overwrites the memory allocated for `foo` + +3. **Demonstrating the Issue**: + - The `append` function ultimately calls `write`, which calculates the destination address in memory and writes the data. + - This write corrupts adjacent memory, i.e `foo.length` + + +Run the test in foundry's debugger using + +``` +forge test --match-contract BufferTest --debug test_MemoryCorruption +``` + +and examine in details how this memory corruption happens + +## Mitigating the issue + +To prevent memory corruption, ensure the allocation accounts for the length of the buffer: + +```solidity +function init(buffer memory buf, uint capacity) internal pure returns(buffer memory) { + if (capacity % 32 != 0) { + capacity += 32 - (capacity % 32); + } + buf.capacity = capacity; + assembly { + let ptr := mload(0x40) + mstore(buf, ptr) + mstore(ptr, 0) + mstore(0x40, add(32, add(ptr, capacity))) + } + return buf; +} +``` + +Applying the suggested fix results in `foo.length` being written to `0x140` which prevents the memory corruption. + + +## Sources +- Heavily borrowed from: [Heading Memory Corruption Due To Insufficient Allocation](https://dacian.me/solidity-inline-assembly-vulnerabilities#heading-memory-corruption-due-to-insufficient-allocation) +- [Slot Confusion Leading to Corrupted Reserve Amounts](https://solodit.xyz/issues/m-02-due-to-slot-confusion-reserve-amounts-in-the-pump-will-be-corrupted-resulting-in-wrong-oracle-values-code4rena-basin-basin-git) +- [Bit Shifting Errors Leading to Corrupted Reserve Amounts](https://solodit.xyz/issues/m-03-due-to-bit-shifting-errors-reserve-amounts-in-the-pump-will-be-corrupted-resulting-in-wrong-oracle-values-code4rena-basin-basin-git) +- [ENS Audit](https://solodit.xyz/issues/memory-corruption-in-buffer-fixed-consensys-ens-permanent-registrar-markdown) From 851133ff80d2b50c31a2ca0754d4435f749ada3b Mon Sep 17 00:00:00 2001 From: Nabil Omar <63709472+indeqs@users.noreply.github.com> Date: Thu, 13 Jun 2024 09:52:44 +0000 Subject: [PATCH 2/7] Tidying up memory-corruption-due-to-insufficient-allocation.md for clarity and giving a general description rather than the specific one to the ENS audit --- ...rruption-due-to-insufficient-allocation.md | 45 ++++++++++++++----- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/vulnerabilities/memory-corruption-due-to-insufficient-allocation.md b/vulnerabilities/memory-corruption-due-to-insufficient-allocation.md index ed9acc7..936a7cd 100644 --- a/vulnerabilities/memory-corruption-due-to-insufficient-allocation.md +++ b/vulnerabilities/memory-corruption-due-to-insufficient-allocation.md @@ -1,10 +1,13 @@ -# Memory Corruption Due to Insufficient Memory Allocation +# Data Corruption Due to Insufficient Memory Allocation -Memory corruption occurs when a program writes outside the bounds of allocated memory, potentially overwriting other important data. +Data corruption occurs when a program writes outside the bounds of allocated memory, potentially overwriting other important data. -In Solidity, especially when using inline assembly, it's crucial to allocate enough memory to avoid memory corruption issues. +In Solidity, especially when using inline assembly, it's crucial to allocate enough memory to avoid memory corruption issues. This is particularly critical when using inline assembly, where manual memory management is required. + +Below is a simplified example from [this](https://solodit.xyz/issues/memory-corruption-in-buffer-fixed-consensys-ens-permanent-registrar-markdown) Ethereum Name Service(ENS) audit highlighting how insufficient memory allocation can lead to memory corruption: + +Consider a library `Buffer` that is used to manage a dynamically-sized byte array (`bytes`) with a specific capacity. The library contains functions to initialize the buffer (`init`), add data (`append`) and write data to the buffer (`write`) -Below is an example heavily borrowed off and simplified from [this](https://solodit.xyz/issues/memory-corruption-in-buffer-fixed-consensys-ens-permanent-registrar-markdown) Ethereum Name Service(ENS) audit to demonstrate the issue: ```solidity // SPDX-License-Identifier: MIT @@ -87,7 +90,7 @@ library Buffer { } ``` -A library `Buffer` is used to manage a dynamically-sized byte array (`bytes`) with a specific capacity. The library contains functions to initialize the buffer (`init`), add data (`append`) and write data to the buffer (`write`) +To demonstrate how the corruption occurs, consider the below test contract from the amazing work of [Dacian](https://x.com/DevDacian): ```solidity @@ -119,9 +122,8 @@ contract BufferTest is Test { } ``` -- The above foundry test is borrowed from the amazing work of [Dacian](https://x.com/DevDacian) -### Memory Corruption Explained +### Explanation: 1. **Initialization**: - `Buffer::init` initializes a buffer with a specified capacity. @@ -145,9 +147,9 @@ forge test --match-contract BufferTest --debug test_MemoryCorruption and examine in details how this memory corruption happens -## Mitigating the issue +## Remediation Strategies -To prevent memory corruption, ensure the allocation accounts for the length of the buffer: +1. Ensure Proper Memory Allocation: Always allocate sufficient memory for your data structures. Manage memory to ensure enough space is allocated. To mitigate the ENS data corruption vulnerability discussed above, the below can be done which ensures the memory allocation accounts for the length of the buffer: ```solidity function init(buffer memory buf, uint capacity) internal pure returns(buffer memory) { @@ -165,7 +167,30 @@ function init(buffer memory buf, uint capacity) internal pure returns(buffer mem } ``` -Applying the suggested fix results in `foo.length` being written to `0x140` which prevents the memory corruption. +2. Align Memory Allocations: Align memory allocations to 32-byte boundaries to ensure proper memory access and avoid unaligned writes, which can lead to memory corruption, e.g + +```solidity +function alignedAlloc(uint capacity) internal pure returns (bytes memory) { + if (capacity % 32 != 0) { + capacity += 32 - (capacity % 32); + } + bytes memory buffer = new bytes(capacity); + return buffer; +} +``` + +3. Perform Bounds Checks: Perform bounds checks before writing data to ensure that you do not write outside the allocated memory. This can prevent out-of-bounds writes and potential memory corruption, e.g + +```solidity +function writeData(bytes memory buffer, uint offset, bytes32 data, uint len) internal pure { + require(offset + len <= buffer.length, "Write out of bounds"); + assembly { + let bufptr := add(buffer, 32) // Skip the length field + let dest := add(bufptr, offset) + mstore(dest, data) + } +} +``` ## Sources From 1c40699a9e92ada113f207092a019fa004ee61b0 Mon Sep 17 00:00:00 2001 From: Nabil Omar <63709472+indeqs@users.noreply.github.com> Date: Thu, 13 Jun 2024 09:59:55 +0000 Subject: [PATCH 3/7] Update README.md --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 477667a..a7bc07c 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ - [Shadowing State Variables](./vulnerabilities/shadowing-state-variables.md) - [Weak Sources of Randomness from Chain Attributes](./vulnerabilities/weak-sources-randomness.md) - [Missing Protection against Signature Replay Attacks](./vulnerabilities/missing-protection-signature-replay.md) -- [Memory Corruption Due To Insufficient Memory Allocation](./vulnerabilities/memory-corruption-due-to-insufficient-allocation.md) +- [Data Corruption Due To Insufficient Memory Allocation](./vulnerabilities/memory-corruption-due-to-insufficient-allocation.md) - [Requirement Validation](./vulnerabilities/requirement-violation.md) - [Write to Arbitrary Storage Location](./vulnerabilities/arbitrary-storage-location.md) - [Hash Collision when using abi.encodePacked() with Multiple Variable-Length Arguments](./vulnerabilities/hash-collision.md) From 1828739b19ec6d152301fcfa1573cb99ffee68b2 Mon Sep 17 00:00:00 2001 From: konfushon Date: Thu, 13 Jun 2024 13:02:16 +0300 Subject: [PATCH 4/7] Renamed from memory-corruption-due-to-insufficient-memory-allocation.md to data-corruption-due-to-insufficient-memory-allocation.md --- ...n-due-to-insufficient-memory-allocation.md | 200 ++++++++++++++++++ 1 file changed, 200 insertions(+) create mode 100644 vulnerabilities/data-corruption-due-to-insufficient-memory-allocation.md diff --git a/vulnerabilities/data-corruption-due-to-insufficient-memory-allocation.md b/vulnerabilities/data-corruption-due-to-insufficient-memory-allocation.md new file mode 100644 index 0000000..936a7cd --- /dev/null +++ b/vulnerabilities/data-corruption-due-to-insufficient-memory-allocation.md @@ -0,0 +1,200 @@ +# Data Corruption Due to Insufficient Memory Allocation + +Data corruption occurs when a program writes outside the bounds of allocated memory, potentially overwriting other important data. + +In Solidity, especially when using inline assembly, it's crucial to allocate enough memory to avoid memory corruption issues. This is particularly critical when using inline assembly, where manual memory management is required. + +Below is a simplified example from [this](https://solodit.xyz/issues/memory-corruption-in-buffer-fixed-consensys-ens-permanent-registrar-markdown) Ethereum Name Service(ENS) audit highlighting how insufficient memory allocation can lead to memory corruption: + +Consider a library `Buffer` that is used to manage a dynamically-sized byte array (`bytes`) with a specific capacity. The library contains functions to initialize the buffer (`init`), add data (`append`) and write data to the buffer (`write`) + + +```solidity +// SPDX-License-Identifier: MIT +pragma solidity 0.8.25; + +import {Test} from "forge-std/Test.sol"; + +// finding : https://solodit.xyz/issues/memory-corruption-in-buffer-fixed-consensys-ens-permanent-registrar-markdown +// code from : https://github.com/ensdomains/buffer/blob/c06d796e2f6086473d229a96c2eb75053a19b8ec/contracts/Buffer.sol + +library Buffer { + + struct buffer { + bytes buf; + uint capacity; + } + + function init(buffer memory buf, uint capacity) internal pure returns(buffer memory) { + if (capacity % 32 != 0) { + capacity += 32 - (capacity % 32); + } + // Allocate space for the buffer data + buf.capacity = capacity; + + assembly { + let ptr := mload(0x40) + mstore(buf, ptr) + mstore(ptr, 0) + + // does not account for length 0x20 (32), even though the `write` function does + mstore(0x40, add(ptr, capacity)) + /// @audit fix: mstore(0x40, add(32, add(ptr, capacity))) + } + return buf; + } + + function append(buffer memory buf, bytes memory data) internal pure returns (buffer memory) { + return write(buf, buf.buf.length, bytes32(data), data.length); + } + + function write(buffer memory buf, uint off, bytes32 data, uint len) private pure returns(buffer memory) { + if (len + off > buf.capacity) { + resize(buf, max(buf.capacity, len) * 2); + } + + uint mask = 256 ** len - 1; + // Right-align data + data = data >> (8 * (32 - len)); + + assembly { + // Memory address of the buffer data + let bufptr := mload(buf) + + // Address = buffer address + sizeof(buffer length) + off + len + let dest := add(add(bufptr, off), len) + + mstore(dest, or(and(mload(dest), not(mask)), data)) + // Update buffer length if we extended it + + if gt(add(off, len), mload(bufptr)) { + mstore(bufptr, add(off, len)) + } + } + return buf; + } + + function resize(buffer memory buf, uint capacity) private pure { + bytes memory oldbuf = buf.buf; + init(buf, capacity); + append(buf, oldbuf); + } + + function max(uint a, uint b) private pure returns(uint) { + if (a > b) { + return a; + } + return b; + } + +} +``` + +To demonstrate how the corruption occurs, consider the below test contract from the amazing work of [Dacian](https://x.com/DevDacian): + + +```solidity +contract BufferTest is Test { + using Buffer for Buffer.buffer; + + function test_MemoryCorruption() external { + Buffer.buffer memory buffer; + buffer.init(1); + + // `foo` immediately follows buffer.buf in memory + bytes memory foo = new bytes(0x01); + + // sanity check passes + assert(1 == foo.length); + + // append "A" 0x41 (65) to buffer. This gets written to the high order byte of `foo.length` + buffer.append("A"); + + /** + * foo.length == 0x4100000000000000000000000000000000000000000000000000000000000001 + * == 29400335157912315244266070412362164103369332044010299463143527189509193072641 + * the below assertion passes showing the memory corruption + */ + + assertEq(29400335157912315244266070412362164103369332044010299463143527189509193072641, foo.length); + + } +} +``` + + +### Explanation: + +1. **Initialization**: + - `Buffer::init` initializes a buffer with a specified capacity. + - Memory allocation is done using `mstore`, and the Free Memory Pointer Address (FMPA) is updated. + +2. **Memory Corruption**: + - In the test, `foo` is a new byte array created right after the buffer in memory. + - When `buffer.append("A")` is called, it writes data into the buffer. + - Due to insufficient memory allocation, this write operation overwrites the memory allocated for `foo` + +3. **Demonstrating the Issue**: + - The `append` function ultimately calls `write`, which calculates the destination address in memory and writes the data. + - This write corrupts adjacent memory, i.e `foo.length` + + +Run the test in foundry's debugger using + +``` +forge test --match-contract BufferTest --debug test_MemoryCorruption +``` + +and examine in details how this memory corruption happens + +## Remediation Strategies + +1. Ensure Proper Memory Allocation: Always allocate sufficient memory for your data structures. Manage memory to ensure enough space is allocated. To mitigate the ENS data corruption vulnerability discussed above, the below can be done which ensures the memory allocation accounts for the length of the buffer: + +```solidity +function init(buffer memory buf, uint capacity) internal pure returns(buffer memory) { + if (capacity % 32 != 0) { + capacity += 32 - (capacity % 32); + } + buf.capacity = capacity; + assembly { + let ptr := mload(0x40) + mstore(buf, ptr) + mstore(ptr, 0) + mstore(0x40, add(32, add(ptr, capacity))) + } + return buf; +} +``` + +2. Align Memory Allocations: Align memory allocations to 32-byte boundaries to ensure proper memory access and avoid unaligned writes, which can lead to memory corruption, e.g + +```solidity +function alignedAlloc(uint capacity) internal pure returns (bytes memory) { + if (capacity % 32 != 0) { + capacity += 32 - (capacity % 32); + } + bytes memory buffer = new bytes(capacity); + return buffer; +} +``` + +3. Perform Bounds Checks: Perform bounds checks before writing data to ensure that you do not write outside the allocated memory. This can prevent out-of-bounds writes and potential memory corruption, e.g + +```solidity +function writeData(bytes memory buffer, uint offset, bytes32 data, uint len) internal pure { + require(offset + len <= buffer.length, "Write out of bounds"); + assembly { + let bufptr := add(buffer, 32) // Skip the length field + let dest := add(bufptr, offset) + mstore(dest, data) + } +} +``` + + +## Sources +- Heavily borrowed from: [Heading Memory Corruption Due To Insufficient Allocation](https://dacian.me/solidity-inline-assembly-vulnerabilities#heading-memory-corruption-due-to-insufficient-allocation) +- [Slot Confusion Leading to Corrupted Reserve Amounts](https://solodit.xyz/issues/m-02-due-to-slot-confusion-reserve-amounts-in-the-pump-will-be-corrupted-resulting-in-wrong-oracle-values-code4rena-basin-basin-git) +- [Bit Shifting Errors Leading to Corrupted Reserve Amounts](https://solodit.xyz/issues/m-03-due-to-bit-shifting-errors-reserve-amounts-in-the-pump-will-be-corrupted-resulting-in-wrong-oracle-values-code4rena-basin-basin-git) +- [ENS Audit](https://solodit.xyz/issues/memory-corruption-in-buffer-fixed-consensys-ens-permanent-registrar-markdown) From 47276eff281b20a4274f87e2c7b7d35bb89c1941 Mon Sep 17 00:00:00 2001 From: Nabil Omar <63709472+indeqs@users.noreply.github.com> Date: Thu, 13 Jun 2024 10:06:36 +0000 Subject: [PATCH 5/7] Update README.md --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index a7bc07c..3a17994 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ - [Shadowing State Variables](./vulnerabilities/shadowing-state-variables.md) - [Weak Sources of Randomness from Chain Attributes](./vulnerabilities/weak-sources-randomness.md) - [Missing Protection against Signature Replay Attacks](./vulnerabilities/missing-protection-signature-replay.md) -- [Data Corruption Due To Insufficient Memory Allocation](./vulnerabilities/memory-corruption-due-to-insufficient-allocation.md) +- [Data Corruption Due To Insufficient Memory Allocation](./vulnerabilities/data-corruption-due-to-insufficient-memory-allocation.md) - [Requirement Validation](./vulnerabilities/requirement-violation.md) - [Write to Arbitrary Storage Location](./vulnerabilities/arbitrary-storage-location.md) - [Hash Collision when using abi.encodePacked() with Multiple Variable-Length Arguments](./vulnerabilities/hash-collision.md) From 164359570ec69b120a48842992ff43edd4a7526b Mon Sep 17 00:00:00 2001 From: Nabil Omar <63709472+indeqs@users.noreply.github.com> Date: Thu, 13 Jun 2024 10:07:24 +0000 Subject: [PATCH 6/7] Delete vulnerabilities/memory-corruption-due-to-insufficient-allocation.md --- ...rruption-due-to-insufficient-allocation.md | 200 ------------------ 1 file changed, 200 deletions(-) delete mode 100644 vulnerabilities/memory-corruption-due-to-insufficient-allocation.md diff --git a/vulnerabilities/memory-corruption-due-to-insufficient-allocation.md b/vulnerabilities/memory-corruption-due-to-insufficient-allocation.md deleted file mode 100644 index 936a7cd..0000000 --- a/vulnerabilities/memory-corruption-due-to-insufficient-allocation.md +++ /dev/null @@ -1,200 +0,0 @@ -# Data Corruption Due to Insufficient Memory Allocation - -Data corruption occurs when a program writes outside the bounds of allocated memory, potentially overwriting other important data. - -In Solidity, especially when using inline assembly, it's crucial to allocate enough memory to avoid memory corruption issues. This is particularly critical when using inline assembly, where manual memory management is required. - -Below is a simplified example from [this](https://solodit.xyz/issues/memory-corruption-in-buffer-fixed-consensys-ens-permanent-registrar-markdown) Ethereum Name Service(ENS) audit highlighting how insufficient memory allocation can lead to memory corruption: - -Consider a library `Buffer` that is used to manage a dynamically-sized byte array (`bytes`) with a specific capacity. The library contains functions to initialize the buffer (`init`), add data (`append`) and write data to the buffer (`write`) - - -```solidity -// SPDX-License-Identifier: MIT -pragma solidity 0.8.25; - -import {Test} from "forge-std/Test.sol"; - -// finding : https://solodit.xyz/issues/memory-corruption-in-buffer-fixed-consensys-ens-permanent-registrar-markdown -// code from : https://github.com/ensdomains/buffer/blob/c06d796e2f6086473d229a96c2eb75053a19b8ec/contracts/Buffer.sol - -library Buffer { - - struct buffer { - bytes buf; - uint capacity; - } - - function init(buffer memory buf, uint capacity) internal pure returns(buffer memory) { - if (capacity % 32 != 0) { - capacity += 32 - (capacity % 32); - } - // Allocate space for the buffer data - buf.capacity = capacity; - - assembly { - let ptr := mload(0x40) - mstore(buf, ptr) - mstore(ptr, 0) - - // does not account for length 0x20 (32), even though the `write` function does - mstore(0x40, add(ptr, capacity)) - /// @audit fix: mstore(0x40, add(32, add(ptr, capacity))) - } - return buf; - } - - function append(buffer memory buf, bytes memory data) internal pure returns (buffer memory) { - return write(buf, buf.buf.length, bytes32(data), data.length); - } - - function write(buffer memory buf, uint off, bytes32 data, uint len) private pure returns(buffer memory) { - if (len + off > buf.capacity) { - resize(buf, max(buf.capacity, len) * 2); - } - - uint mask = 256 ** len - 1; - // Right-align data - data = data >> (8 * (32 - len)); - - assembly { - // Memory address of the buffer data - let bufptr := mload(buf) - - // Address = buffer address + sizeof(buffer length) + off + len - let dest := add(add(bufptr, off), len) - - mstore(dest, or(and(mload(dest), not(mask)), data)) - // Update buffer length if we extended it - - if gt(add(off, len), mload(bufptr)) { - mstore(bufptr, add(off, len)) - } - } - return buf; - } - - function resize(buffer memory buf, uint capacity) private pure { - bytes memory oldbuf = buf.buf; - init(buf, capacity); - append(buf, oldbuf); - } - - function max(uint a, uint b) private pure returns(uint) { - if (a > b) { - return a; - } - return b; - } - -} -``` - -To demonstrate how the corruption occurs, consider the below test contract from the amazing work of [Dacian](https://x.com/DevDacian): - - -```solidity -contract BufferTest is Test { - using Buffer for Buffer.buffer; - - function test_MemoryCorruption() external { - Buffer.buffer memory buffer; - buffer.init(1); - - // `foo` immediately follows buffer.buf in memory - bytes memory foo = new bytes(0x01); - - // sanity check passes - assert(1 == foo.length); - - // append "A" 0x41 (65) to buffer. This gets written to the high order byte of `foo.length` - buffer.append("A"); - - /** - * foo.length == 0x4100000000000000000000000000000000000000000000000000000000000001 - * == 29400335157912315244266070412362164103369332044010299463143527189509193072641 - * the below assertion passes showing the memory corruption - */ - - assertEq(29400335157912315244266070412362164103369332044010299463143527189509193072641, foo.length); - - } -} -``` - - -### Explanation: - -1. **Initialization**: - - `Buffer::init` initializes a buffer with a specified capacity. - - Memory allocation is done using `mstore`, and the Free Memory Pointer Address (FMPA) is updated. - -2. **Memory Corruption**: - - In the test, `foo` is a new byte array created right after the buffer in memory. - - When `buffer.append("A")` is called, it writes data into the buffer. - - Due to insufficient memory allocation, this write operation overwrites the memory allocated for `foo` - -3. **Demonstrating the Issue**: - - The `append` function ultimately calls `write`, which calculates the destination address in memory and writes the data. - - This write corrupts adjacent memory, i.e `foo.length` - - -Run the test in foundry's debugger using - -``` -forge test --match-contract BufferTest --debug test_MemoryCorruption -``` - -and examine in details how this memory corruption happens - -## Remediation Strategies - -1. Ensure Proper Memory Allocation: Always allocate sufficient memory for your data structures. Manage memory to ensure enough space is allocated. To mitigate the ENS data corruption vulnerability discussed above, the below can be done which ensures the memory allocation accounts for the length of the buffer: - -```solidity -function init(buffer memory buf, uint capacity) internal pure returns(buffer memory) { - if (capacity % 32 != 0) { - capacity += 32 - (capacity % 32); - } - buf.capacity = capacity; - assembly { - let ptr := mload(0x40) - mstore(buf, ptr) - mstore(ptr, 0) - mstore(0x40, add(32, add(ptr, capacity))) - } - return buf; -} -``` - -2. Align Memory Allocations: Align memory allocations to 32-byte boundaries to ensure proper memory access and avoid unaligned writes, which can lead to memory corruption, e.g - -```solidity -function alignedAlloc(uint capacity) internal pure returns (bytes memory) { - if (capacity % 32 != 0) { - capacity += 32 - (capacity % 32); - } - bytes memory buffer = new bytes(capacity); - return buffer; -} -``` - -3. Perform Bounds Checks: Perform bounds checks before writing data to ensure that you do not write outside the allocated memory. This can prevent out-of-bounds writes and potential memory corruption, e.g - -```solidity -function writeData(bytes memory buffer, uint offset, bytes32 data, uint len) internal pure { - require(offset + len <= buffer.length, "Write out of bounds"); - assembly { - let bufptr := add(buffer, 32) // Skip the length field - let dest := add(bufptr, offset) - mstore(dest, data) - } -} -``` - - -## Sources -- Heavily borrowed from: [Heading Memory Corruption Due To Insufficient Allocation](https://dacian.me/solidity-inline-assembly-vulnerabilities#heading-memory-corruption-due-to-insufficient-allocation) -- [Slot Confusion Leading to Corrupted Reserve Amounts](https://solodit.xyz/issues/m-02-due-to-slot-confusion-reserve-amounts-in-the-pump-will-be-corrupted-resulting-in-wrong-oracle-values-code4rena-basin-basin-git) -- [Bit Shifting Errors Leading to Corrupted Reserve Amounts](https://solodit.xyz/issues/m-03-due-to-bit-shifting-errors-reserve-amounts-in-the-pump-will-be-corrupted-resulting-in-wrong-oracle-values-code4rena-basin-basin-git) -- [ENS Audit](https://solodit.xyz/issues/memory-corruption-in-buffer-fixed-consensys-ens-permanent-registrar-markdown) From a41bd79a5a1991b1600d40bcbead289b43224b23 Mon Sep 17 00:00:00 2001 From: Nabil Omar <63709472+indeqs@users.noreply.github.com> Date: Thu, 13 Jun 2024 10:08:16 +0000 Subject: [PATCH 7/7] Update data-corruption-due-to-insufficient-memory-allocation.md --- .../data-corruption-due-to-insufficient-memory-allocation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vulnerabilities/data-corruption-due-to-insufficient-memory-allocation.md b/vulnerabilities/data-corruption-due-to-insufficient-memory-allocation.md index 936a7cd..581a3e2 100644 --- a/vulnerabilities/data-corruption-due-to-insufficient-memory-allocation.md +++ b/vulnerabilities/data-corruption-due-to-insufficient-memory-allocation.md @@ -2,7 +2,7 @@ Data corruption occurs when a program writes outside the bounds of allocated memory, potentially overwriting other important data. -In Solidity, especially when using inline assembly, it's crucial to allocate enough memory to avoid memory corruption issues. This is particularly critical when using inline assembly, where manual memory management is required. +In Solidity, especially when using inline assembly, it's crucial to allocate enough memory to avoid data corruption issues. This is particularly critical when using inline assembly, where manual memory management is required. Below is a simplified example from [this](https://solodit.xyz/issues/memory-corruption-in-buffer-fixed-consensys-ens-permanent-registrar-markdown) Ethereum Name Service(ENS) audit highlighting how insufficient memory allocation can lead to memory corruption: