Use assembly to clean bytes in LibAddress

[reednaa]

Description

After an internal discussion at LI.FI we couldn't settle on what direct casting in Solidity actually does. It seems casting in Solidity is fairly imprecise regarding cleaning bytes after casting.

To ensure that no mistakes are made when casting, variable cleaning is now explicitly done in assembly.

No idea what happened regarding the gas snapshots.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[luiz-lvj]

Hey @reednaa, in general I think including assembly adds some complexity to readers, so I believe it would be better to keep the cast as address(uint160(uint256(identifier))). According, to the docs on type conversion:

If an integer is explicitly converted to a smaller type, higher-order bits are cut off:
uint32 a = 0x12345678;
uint16 b = uint16(a); // b will be 0x5678 now

And also the docs on casting to the type address:

As described in Address Literals, hex literals of the correct size that pass the checksum test are of address type. No other literals can be implicitly converted to the address type.
Explicit conversions to address are allowed only from bytes20 and uint160.

So, according to the docs, the effect of address(uint160(uint256(identifier))) is the same as using assembly, so I'd prefer to keep casting.

[reednaa]

This is what we internally initially also concluded.

However, it is only true at usage time. When you use a smaller type, then its higher bits are cut off:

uint256 v = type(uint256).max; // 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
address casted = address(uint160(uint256(v)));
bytes memory b = abi.encodePacked(casted); // 0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF
b = abi.encode(casted); // 0x000000000000000000000000ffffffffffffffffffffffffffffffffffffffff

All as you would expect, however, if we access casted in assembly before using it, then we get

uint256 v = type(uint256).max; // 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
address casted = address(uint160(uint256(v)))
uint256 va;
assembly {
    va := casted // 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
}

Since the codebase uses assembly values, it is much safer to do this casting in assembly land rather than in Solidity land to ensure they are all casted properly.

[luiz-lvj]

That's interesting. I can see this becoming an issue with heavier usage of assembly and would be an improper casting. We could argue that's an edge case to do this back-and-forth with address and uint256 bytes, and at some point the bytes should be assumed dirty. Assembly seems like a good solution to it.

[reednaa]

(Change casting function names to asSanitisedAddress)

[frangio]

After reviewing this my thoughts are:

• This PR is not necessary. It's safe to produce an address without cleaning upper bits. The responsibility is on the consumer of an address to clean it, if necessary, as per Solidity's documentation and recommendations.
• If we want to ensure the value is clean for additional safety, in case a consumer (such as an assembly block in this repo) needs and forgets to clean it, I can see that this PR provides a layer of mitigation but it's not sufficient protection, the consumer always has to clean it.
  • Solidity claims that IR now always cleans values it produces, so merging this PR would be aligned with that. This is perhaps a good argument to merge.
  • That said, whatever Solidity-native casting does should be safe. This is an argument against merging. 🙂
• If we want the PR anyway, I'd prefer not to rename to asSanitisedAddress, since an address should never be assumed sanitised anyway.