code423n4 commented 1 year ago

Lines of code

https://github.com/ProjectOpenSea/seaport/blob/5de7302bc773d9821ba4759e47fc981680911ea0/contracts/lib/OrderValidator.sol#L151-L158

Vulnerability details

Impact

Detailed description of the impact of this finding. _validateOrderAndUpdateStatus() fails to check the condition that the numerator and denominator are both equal to 1

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. _validateOrderAndUpdateStatus() fails to check the condition that the numerator and denominator are both equal to 1, see below:

// Ensure that the numerator and denominator are both equal to 1.
            assembly {
                // (1 ^ nd =/= 0) => (nd =/= 1) => (n =/= 1) || (d =/= 1)
                // It's important that the values are 120-bit masked before
                // multiplication is applied. Otherwise, the last implication
                // above is not correct (mod 2^256).
                invalidFraction := xor(mul(numerator, denominator), 1)
            }

https://github.com/ProjectOpenSea/seaport/blob/5de7302bc773d9821ba4759e47fc981680911ea0/contracts/lib/OrderValidator.sol#L151-L158

Tools Used

Remix

Recommended Mitigation Steps

We reimplement it as follows:

// Ensure that the numerator and denominator are both equal to 1.
invalidFraction := numerator == 1 &&  denominator == 1;

0age commented 1 year ago

contested; the numerator and denominator are masked a few lines above so when multiplied they cannot overflow, meaning they must both be equal to 1 for the xor condition to hold after multiplying.

c4-judge commented 1 year ago

HickupHH3 marked the issue as unsatisfactory: Invalid

HickupHH3 commented 1 year ago

yeah, the referenced code does the exact check that the warden's saying isn't. with the 120-bit masking to both the numerator and denominator prior.

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