Project
One World
NFTDeFi
15,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Wrong memory access in _msgSender()

sabit

Summary

Wrong memory access in _msgSender().

Vulnerability Details

When msg.data is copied to memory as 'array':

  • First 32 bytes: stores the length of the array

  • Next bytes: contains the actual calldata

function _msgSender()
internal
view
override
returns (address sender)
{
if (msg.sender == address(this)) {
bytes memory array = msg.data;
uint256 index = msg.data.length;
assembly {
// Load the 32 bytes word from memory with the address on the lower 20 bytes, and mask those.
sender := and(
mload(add(array, index)),
0xffffffffffffffffffffffffffffffffffffffff
)
}
} else {
sender = msg.sender;
}
return sender;
}

https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/dao/MembershipFactory.sol#L182C4-L202C6

Setting index to msg.data.length means we're pointing to the end of the array.

bytes memory array = msg.data;
uint256 index = msg.data.length; // Problem starts here
assembly {
sender := and(
mload(add(array, index)),
0xffffffffffffffffffffffffffffffffffffffff
)
}
​

For example:
If msg.data is 100 bytes long:

  • Setting index = msg.data.length means index = 100

  • This points to position 100 in memory

  • But the valid data only exists from position 0 to 99

  • Position 100 is already outside of the array



    The mload operation reads 32 bytes starting from this position. This reads beyond the allocated memory space. It could be reading from:

  • Uninitialized memory

  • Memory from other operations

  • Sensitive data from other contract operations

In the _msgSender() function, this means we're attempting to read 32 bytes starting from a position that's already beyond the valid data, which is unsafe and can lead to reading unrelated memory content.

Here's a breakdown of what the Assembly code is doing:

assembly {
// Step 1: add(array, index)
// - Takes our array pointer
// - Adds the index value to it
// - Creates a new memory position to read from
​
// Step 2: mload(...)
// - Reads 32 bytes from that memory position
// - Returns these bytes as a single word
​
// Step 3: and(..., 0xffffffffffffffffffffffffffffffffffffffff)
// - Takes those 32 bytes we read
// - Applies the mask to keep only the last 20 bytes (address size)
// - Discards any extra bytes beyond address length
​
sender := and(
mload(add(array, index)),
0xffffffffffffffffffffffffffffffffffffffff
)
}
​

Impact

When _msgSender() is the contract address, wrong value would be stored as sender and this could cause different issues depending on what is read outside of the allocated space.

Tools Used

Manual review

Recommendations

Value should be read within the allocated bounds:

function _msgSender() internal view override returns (address) {
address sender = msg.sender;
if (sender == address(this)) {
bytes memory array = msg.data;
// Read the last 20 bytes of valid data
uint256 index = array.length - 20;
assembly {
sender := and(
mload(add(add(array, 0x20), index)),
0xffffffffffffffffffffffffffffffffffffffff
)
}
}
return sender;
}
​
Updates

Lead Judging Commences

0xbrivan2 Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
0xbrivan2 Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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