TypedMemView memory-safety guarantees are broken

[nikitastupin]

Hey @prestwich @ErinHales!

I've found an interesting bug in the isValid function of the TypedMemView library.

function isValid(bytes29 memView) internal pure returns (bool ret) {
    if (typeOf(memView) == 0xffffffffff) {return false;}
    uint256 _end = end(memView);
    assembly {
        // solium-disable-previous-line security/no-inline-assembly
        ret := not(gt(_end, mload(0x40)))
    }
}

The function is expected to return true when the memView data structure points to an allocated memory and false otherwise.

However, the function always returns true because the not operator does bitwise not and anything greater than zero is converted to the boolean value of true. Thus whatever the result of the gt operator is (zero or one) the functions returns true.

Impact

The isValid function is used in multiple other functions throughout the library and used to ensure the memory-safety guarantees of the library. Since the function always returns true these memory-safety guarantees break.

Unfortunately, I didn't have enough time&energy now to explore the impact on the Nomad project, where the library is used extensively, or other projects affected by the bug (some of which are quite popular: https://github.com/summa-tx/memview-sol/network/dependents).

Recommendation

Use the iszero operator instead of the not operator to invert a boolean value in assembly.

References

• https://docs.soliditylang.org/en/v0.8.18/yul.html#evm-dialect

Proof of Concept

Here is a toy example to test this (you may copy-paste it to Remix IDE, deploy, and execute there).

pragma solidity 0.8.18;

contract poc {
     function f() external pure returns (bool o0, bool o1, bool o2) {
         assembly {
            o0 := not(0)
            o1 := not(1)
            o2 := not(1337)
         }
         require(o0);
         require(o1);
         require(o2);
     }
}

The f functions returns three true values.

[prestwich]

Thanks for the report!

In summary, isValid will return false positives when passed a view whose end is outside allocated memory.

I believe this can only be triggered via manual memory modification by the library consumer. I.e. there's no way for anyone to run into this without unsafely modifying memory or views on their own. The library does not expose any interface for the user to construct views where the end overruns allocated memory. While the unsafe* methods will build these for internal use, they are private and cannot be accessed by library consumers

Because of this, the safety guarantees seem to hold as long as solidity's memory model is respected. I believe this can't be a source of memory unsafety. I'd assign this low severity. As this is clearly a bug. And unsafe behavior, even under unsafe circumstances, is undesirable. I've pushed a fix, and will publish a patch to npm shortly

I've pushed afc2021 with a fix to main, and applied the same fix to #6

[nikitastupin]

Thanks your for fast reply and merging the change! I agree with the severity because I couldn't come up with a way to exploit this 🤷 thanks for giving additional details!