Platform: Code4rena
Start Date: 13/01/2023
Pot Size: $100,500 USDC
Total HM: 1
Participants: 23
Period: 10 days
Judge: hickuphh3
Total Solo HM: 1
Id: 201
League: ETH
Rank: 1/23
Findings: 1
Award: $71,500.00
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: 0xsomeone
71500 USDC - $71,500.00
The order hashes are incorrectly encoded during the _encodeOrderHashes mechanism, causing functions such as _encodeRatifyOrder and _encodeValidateOrder to misbehave.
The order hashes encoding mechanism appears to be incorrect as the instructions srcLength.next().offset(headAndTailSize) will cause the pointer to move to the end of the array (i.e. next() skips the array's length bitwise entry and offset(headAndTailSize) causes the pointer to point right after the last element). In turn, this will cause the 0x04 precompile within MemoryPointerLib::copy to handle the data incorrectly and attempt to copy data from the srcLength.next().offset(headAndTailSize) pointer onwards which will be un-allocated space and thus lead to incorrect bytes being copied.
Manual inspection of the codebase, documentation of the ETH precompiles, and the Solidity compiler documentation.
We advise the offset instruction to be omitted as the current implementation will copy from unsafe memory space, causing data corruption in the worst-case scenario and incorrect order hashes being specified in the encoded payload. As an additional point, the _encodeOrderHashes will fail execution if the array of order hashes is empty as a headAndTailSize of 0 will cause the MemoryPointerLib::copy function to fail as the precompile would yield a returndatasize() of 0.
#0 - 0age
2023-01-24T23:54:04Z
This is a confirmed issue (though categorizing it as high-risk seems unfair, at worst it just means that zones and contract offerers wouldn't be able to rely on the orderHashes array) and has been fixed here: https://github.com/ProjectOpenSea/seaport/pull/918
#1 - c4-judge
2023-01-25T06:10:32Z
HickupHH3 marked the issue as primary issue
#2 - HickupHH3
2023-01-25T06:16:02Z
Agree that high severity is overstated. Given that it would affect upstream functions (_encodeRatifyOrder and _encodeValidateOrder is called by a few other functions like _assertRestrictedAdvancedOrderValidity()), medium severity would be more appropriate.
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
#3 - c4-judge
2023-01-25T06:16:11Z
HickupHH3 changed the severity to 2 (Med Risk)
#4 - c4-judge
2023-01-25T06:17:27Z
HickupHH3 marked the issue as satisfactory
#5 - c4-judge
2023-01-26T06:31:20Z
HickupHH3 marked the issue as selected for report