-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add data corruption due to insufficient memory allocation #80
Open
indeqs
wants to merge
7
commits into
kadenzipfel:master
Choose a base branch
from
indeqs:memory-corruption-due-to-insufficient-allocation
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
08a0d28
Add memory-corruption-due-to-insufficient-allocation.md
851133f
Tidying up memory-corruption-due-to-insufficient-allocation.md for cl…
indeqs 1c40699
Update README.md
indeqs 1828739
Renamed from memory-corruption-due-to-insufficient-memory-allocation.…
47276ef
Update README.md
indeqs 1643595
Delete vulnerabilities/memory-corruption-due-to-insufficient-allocati…
indeqs a41bd79
Update data-corruption-due-to-insufficient-memory-allocation.md
indeqs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
175 changes: 175 additions & 0 deletions
175
vulnerabilities/memory-corruption-due-to-insufficient-allocation.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be more accurate to say, "Memory corruption can occur..." since it's not the only cause
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough, lemme see if we can generalize this vuln type as suggested