ArkProject: NFT Bridge
ArkProject
NFTBridge
60,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Memory allocation mismatch in getWhiteListedCollections function

sabit

Summary

The getWhiteListedCollections function creates a mismatch between the reported length of a returned array and its actual memory allocation. This discrepancy is caused by improper use of inline assembly to modify the array's length without reallocating memory.

Vulnerability Details

The function allocates a fixed-size memory array based on the total number of collections, but then uses inline assembly to modify only the length field of this array to match the number of whitelisted collections. This changes the reported length of the array without adjusting the actual memory allocation.

function getWhiteListedCollections() external view returns (address[] memory) {
uint256 offset = 0;
uint256 nbElem = _collections.length;
// solidity doesn't support dynamic length array in memory
address[] memory ret = new address[](nbElem);
for (uint256 i = 0; i < nbElem ;++i) {
address cur = _collections[i];
if (_whiteList[cur]) {
ret[offset] = cur;
offset += 1;
}
}
// resize output array
assembly {
mstore(ret, offset)
}
return ret;
}

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L314C5-L332C6

The function creates a memory array with a size equal to the total number of collections. Then populates this array only with whitelisted collections, tracking the count with an 'offset' variable. Using inline assembly, the function overwrites the array's length field with the 'offset' value. And it returns this modified array.

The result is an array that reports a length potentially smaller than its actual memory allocation.

Here's a practical example:

Suppose we have this simplified version of the function:

function getWhiteListedCollections() external view returns (address[] memory) {
address[] memory ret = new address[](5); // Allocate for 5 addresses
uint256 offset = 3; // Pretend we've added 3 whitelisted addresses
ret[0] = 0x1111111111111111111111111111111111111111;
ret[1] = 0x2222222222222222222222222222222222222222;
ret[2] = 0x3333333333333333333333333333333333333333;
​
assembly {
mstore(ret, offset)
}
return ret;
}

Now, let's see what's happening in memory:

  • Initial memory allocation:

[0x0000...0005] // 32 bytes representing length 5
[0x0000...0000] // 32 bytes for address slot 0
[0x0000...0000] // 32 bytes for address slot 1
[0x0000...0000] // 32 bytes for address slot 2
[0x0000...0000] // 32 bytes for address slot 3
[0x0000...0000] // 32 bytes for address slot 4

  • After populating with 3 addresses:

    [0x0000...0005] // 32 bytes representing length 5
    [0x1111...1111] // 32 bytes for address 0x1111...1111
    [0x2222...2222] // 32 bytes for address 0x2222...2222
    [0x3333...3333] // 32 bytes for address 0x3333...3333
    [0x0000...0000] // 32 bytes for address slot 3 (unused)
    [0x0000...0000] // 32 bytes for address slot 4 (unused)

  • After assembly code executes:

[0x0000...0003] // 32 bytes representing length 3 (changed by assembly)
[0x1111...1111] // 32 bytes for address 0x1111...1111
[0x2222...2222] // 32 bytes for address 0x2222...2222
[0x3333...3333] // 32 bytes for address 0x3333...3333
[0x0000...0000] // 32 bytes for address slot 3 (still exists but "hidden")
[0x0000...0000] // 32 bytes for address slot 4 (still exists but "hidden")

Now, when this function returns ret, it's returning an array that:

  • Reports a length of 3

  • Actually has memory allocated for 5 elements

Impact

While the direct impact is mitigated because the function is not used elsewhere in the code, potential risks include:

  • Gas inefficiency: The function always allocates memory for the maximum possible size, which is wasteful if the actual number of whitelisted collections is smaller.

  • Maintenance risks: If the function is used in future updates or by external contracts, it could lead to: a) Exposure of uninitialized memory if accessed beyond the reported length. b) Conflicts with memory management if other operations assume the unused memory is free.

Tools Used

Manual review

Recommendations

The function can be refactored to allocate only the needed memory:

function getWhiteListedCollections() external view returns (address[] memory) {
// First pass: count whitelisted collections
uint256 count = 0;
for (uint256 i = 0; i < _collections.length; i++) {
if (_whiteList[_collections[i]]) {
count++;
}
}
// Allocate array of correct size
address[] memory whitelistedCollections = new address[](count);
// Second pass: populate array
uint256 index = 0;
for (uint256 i = 0; i < _collections.length; i++) {
if (_whiteList[_collections[i]]) {
whitelistedCollections[index] = _collections[i];
index++;
}
}
return whitelistedCollections;
}
Updates

Lead Judging Commences

n0kto Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational / Gas

Please, do not suppose impacts, think about the real impact of the bug and check the CodeHawks documentation to confirm: https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity A PoC always helps to understand the real impact possible.

Appeal created

sabit Submitter
11 months ago
n0kto Lead Judge
11 months ago
n0kto Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational / Gas

Please, do not suppose impacts, think about the real impact of the bug and check the CodeHawks documentation to confirm: https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity A PoC always helps to understand the real impact possible.

Support

FAQs

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