[L-01] Reverting amount calculations could be caused intendedly by previous malicious buyers
Impact
The Out-of-scope section in the documentation says:
As all derived amounts of partial fills and ascending/descending orders need to be derived without integer overflows, some categories of order may end up in a state where they can no longer be fulfilled due to reverting amount calculations.
We wanted to emphasize that reverting amount calculation can be approached intendedly by malicious buyers by partially fulfilling an order with a large numerator and denominator.
This is possible because, currently, the code doesn't check if the provided numerator and denominator are simplified (or reduced). Therefore, if a buyer wants to buy 1/10 of the order, they can submit the numerator and the denominator as X and 10X, where X is allowed to be a significantly larger number that makes the calculation result almost to overflow. As a result, following fulfillments from others may easily fall (because of the overflow revert), but only fulfilling the rest of the order may succeed.
Proof of Concept
At L132-149 in OrderValidator, we can see that the provided numerator and denominator in fulfillment are used directly without being restricted to be in the reduced form.
At L209-211, the provided numerator and denominator are multiplied by the filled numbers, respectively, with a default overflow check. If the filled numbers are near the maximum value of type uint256, the calculation may trigger an overflow and cause the partial fulfillment to fail.
Suggest checking if the provided fraction is in the reduced form by ensuring gcd(numerator, denominator) is 1.
[L-02] Inconsistency between comments and code
Impact
The code sets the recipient to the offerer if the execution amount is 0. However, the code comment says it should be set if the amount is nonzero. The correct logic of the code is correct.
The _name() function returns Seaport without a null character at the end of the string, which may cause some random bytes to be displayed at the end of the string if the front-end app does not append the null byte.
HardlyDifficult
commented
2 years ago
[L-01] Reverting amount calculations could be caused intendedly by previous malicious buyers
Impact
The Out-of-scope section in the documentation says:
We wanted to emphasize that reverting amount calculation can be approached intendedly by malicious buyers by partially fulfilling an order with a large numerator and denominator.
This is possible because, currently, the code doesn't check if the provided numerator and denominator are simplified (or reduced). Therefore, if a buyer wants to buy 1/10 of the order, they can submit the numerator and the denominator as X and 10X, where X is allowed to be a significantly larger number that makes the calculation result almost to overflow. As a result, following fulfillments from others may easily fall (because of the overflow revert), but only fulfilling the rest of the order may succeed.
Proof of Concept
At L132-149 in
OrderValidator, we can see that the provided numerator and denominator in fulfillment are used directly without being restricted to be in the reduced form.At L209-211, the provided numerator and denominator are multiplied by the filled numbers, respectively, with a default overflow check. If the filled numbers are near the maximum value of type
uint256, the calculation may trigger an overflow and cause the partial fulfillment to fail.Reference:
Recommended Mitigation Steps
Suggest checking if the provided fraction is in the reduced form by ensuring gcd(numerator, denominator) is 1.
[L-02] Inconsistency between comments and code
Impact
The code sets the
recipientto theoffererif the execution amount is 0. However, the code comment says it should be set if the amount is nonzero. The correct logic of the code is correct.Proof of Concept
See the code comment at L180.
Reference:
Recommended Mitigation Steps
Change
nonzerotozeroat L180.[L-03] Name without an appended null character
Impact
The
_name()function returnsSeaportwithout a null character at the end of the string, which may cause some random bytes to be displayed at the end of the string if the front-end app does not append the null byte.Proof of Concept
For example, we can see the
Seaport's name on Etherscan is followed by a non-printable character. Link: https://etherscan.io/address/0x00000000006cee72100d161c57ada5bb2be1ca79#readContractReference:
Recommended Mitigation Steps
Return an additional null byte at the end of the string.
[NC-01] Typo in the test
fullfills->fulfillsReference: