Submitted by panprog, also found by alan724 and GimelSec
BytesUtils.sol#L66-L70
Due to incorrect condition in ByteUtil.compare function, irrelevant characters are masked out only for strings shorter than 32 characters. However, they must be masked out for strings of all lengths in the last pass of the loop (when remainder of the string is 32 characters or less). This leads to incorrect comparision of strings longer than 32 characters where len or otherlen is smaller than string length (characters beyond provided length are still accounted for in the comparision in this case while they should be ignored).
This wrong compare behaviour also makes RRUtils.compareNames fail to correctly compare DNS names in certain cases.
While the BytesUtil.compare and RRUtils.compareNames methods are currently not used in the functions in the scope (but are used in mainnets DNSSECImpl.checkNsecName, which is out of scope here), theyre public library functions relied upon and can be used by the other users or the ENS project in the future. And since the functions in scope provide incorrect result, thats a wrong (unexpected) behaviour of the smart contract. Moreover, since the problem can be seen only with the large strings (more than 32 characters), this might go unnoticed with any code that uses compare or compareNames method and can potentially lead to high security risk of any integration project or ENS itself.
Example strings to compare which give incorrect result:
01234567890123450123456789012345ab
01234567890123450123456789012345aa
Each string is 34 characters long, first 33 characters are the same, the last one is different. If we compare first 33 characters of both strings, the result should be equal (as they only differ in the 34th character), but compare will return >, because it fails to ignore the last character of both strings and simply compares strings themselves.
If we compare the first 33 characters from the first string vs all 34 characters of the second string, the result of compare will be >, while the correct result is <, because compare fails to ignore the last character of the first string.
Example dns names to compare which give incorrect result:
01234567890123456789012345678901a0.0123456789012345678901234567890123456789012345678.eth
01234567890123456789012345678901a.0123456789012345678901234567890123456789012345678.eth
The first dns name should come after the second, but dnsCompare returns -1 (the first name to come before), because the length of the 2nd domain (49 characters) is ASCII character 1 and is not correctly masked off during strings comparision.
git diff
https://gist.github.com/panprog/32adefdc853ccd0fd0f1aad85c526bea
then:
yarn test test/dnssec-oracle/TestSolidityTests.js
In addition to the incorrect condition, the mask calculation formula: 32 - shortest + idx will also overflow since shortest can be more than 32, so addition should be performed before subtractions.
Arachnid (ENS) confirmed
