code-423n4 / 2023-01-opensea-findings

0 stars 0 forks source link

Lack of input validation: The contract does not perform any checks on the inputs passed to the functions, which could allow an attacker to pass malicious data and trigger wrong behavior or errors. #112

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/ProjectOpenSea/seaport/blob/5de7302bc773d9821ba4759e47fc981680911ea0/contracts/helpers/PointerLibraries.sol#L1125

Vulnerability details

Impact

https://github.com/ProjectOpenSea/seaport/blob/5de7302bc773d9821ba4759e47fc981680911ea0/contracts/helpers/PointerLibraries.sol#L1125

function readInt208( ReturndataPointer rdPtr ) internal pure returns (int208 value) { assembly { returndatacopy(0, rdPtr, 0x20) value := mload(0) } }

This function reads the int208 at rdPtr in returndata. However, it does not perform any checks on the input rdPtr to ensure that it is within bounds. This could allow an attacker to pass a malicious rdPtr that exceeds the size of returndata, potentially leading to reading or writing arbitrary memory locations.

Proof of Concept

function exploit(ReturndataPointer rdPtr) external { int208 value = readInt208(rdPtr); }

An attacker can call the function exploit and pass an arbitrary value for rdPtr that exceeds the size of returndata, which will cause the readInt208 function to read or write arbitrary memory locations.

Tools Used

Recommended Mitigation Steps

Implement input validation checks on the rdPtr input to ensure that it is within bounds before executing any sensitive operations. Use the built-in Solidity functions require() or assert() to perform the validation checks.

code snippets for example

function readInt208(ReturndataPointer rdPtr) internal pure returns (int208 value) { require(rdPtr <= returndatasize, "Error: ReturndataPointer is out of bounds"); assembly { returndatacopy(0, rdPtr, 0x20) value := mload(0) } }

function readInt216(ReturndataPointer rdPtr) internal pure returns (int216 value) { require(rdPtr <= returndatasize, "Error: ReturndataPointer is out of bounds"); assembly { returndatacopy(0, rdPtr, 0x20) value := mload(0) } }

0age commented 1 year ago

contested; readInt208 is never used by Seaport, and seaport does enforce masking on all input lengths and pointers to prevent reads / writes to arbitrary locations via over or underflow

HickupHH3 commented 1 year ago

Lacking of concrete example(s) of where arbitrary locations were written via over/underflow in codebase.

c4-judge commented 1 year ago

HickupHH3 marked the issue as unsatisfactory: Insufficient proof