Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Facets/AcrossFacetPackedV4.sol
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ contract AcrossFacetPackedV4 is ILiFi, TransferrableOwnership, LiFiData {

// pull tokens from msg.sender
LibAsset.transferFromERC20(
// TODO: migrate to LibBytes.toAddressUnchecked (intentional truncation) — see LibBytes v1.1.0
address(uint160(uint256(sendingAssetId))),
msg.sender,
address(this),
Expand Down Expand Up @@ -229,6 +230,7 @@ contract AcrossFacetPackedV4 is ILiFi, TransferrableOwnership, LiFiData {
) external {
// Deposit assets
LibAsset.transferFromERC20(
// TODO: migrate to LibBytes.toAddressUnchecked (intentional truncation) — see LibBytes v1.1.0
address(uint160(uint256(sendingAssetId))),
msg.sender,
address(this),
Expand Down Expand Up @@ -357,6 +359,7 @@ contract AcrossFacetPackedV4 is ILiFi, TransferrableOwnership, LiFiData {
// Check if receiver is EVM address (20 bytes) or non-EVM (longer)
// If the first 12 bytes are zero, it's likely an EVM address
if (bytes12(data[44:56]) == bytes12(0)) {
// TODO: migrate to LibBytes.toAddressUnchecked (intentional truncation) — see LibBytes v1.1.0
bridgeData.receiver = address(
uint160(uint256(bytes32(data[44:76])))
);
Expand Down Expand Up @@ -398,12 +401,14 @@ contract AcrossFacetPackedV4 is ILiFi, TransferrableOwnership, LiFiData {
}

bridgeData.transactionId = bytes32(data[4:12]); // we truncate intentionally to save gas (not dangerous)
// TODO: migrate to LibBytes.toAddressUnchecked (intentional truncation) — see LibBytes v1.1.0
bridgeData.sendingAssetId = address(
uint160(uint256(bytes32(data[76:108])))
); // sendingAssetId
// Check if receiver is EVM address (20 bytes) or non-EVM (longer)
// If the first 12 bytes are zero, it's likely an EVM address
if (bytes12(data[44:56]) == bytes12(0)) {
// TODO: migrate to LibBytes.toAddressUnchecked (intentional truncation) — see LibBytes v1.1.0
bridgeData.receiver = address(
uint160(uint256(bytes32(data[44:76])))
);
Expand Down
1 change: 1 addition & 0 deletions src/Facets/AcrossFacetV4.sol
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ contract AcrossFacetV4 is
function _convertAddressToBytes32(
address _address
) internal pure returns (bytes32) {
// TODO: migrate to LibBytes.toBytes32 — see LibBytes v1.1.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bare // TODO breadcrumbs.

The 21 breadcrumbs reference "LibBytes v1.1.0" but not the tracking ticket (EXSC-618). Un-ticketed TODOs tend to become permanent — consider referencing the ticket ID so they're greppable and closable, e.g. // TODO(EXSC-618): migrate to LibBytes.toBytes32. Also worth confirming the repo's solhint config doesn't flag TODO, and noting this runs against the repo's "minimal comments" default (though the migration-breadcrumb rationale is defensible).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0fbe4b5 — created a dedicated tracking ticket (EXSC-626) and rewrote all 21 breadcrumbs to // TODO(EXSC-626): migrate to LibBytes.… so they're greppable and closable.

Used a new ticket rather than EXSC-618 since EXSC-618 closes with this PR, whereas the migrations outlive it (each rides its facet's next version bump). .solhint.json has no max-line-length rule, so the longer comments are fine.

return bytes32(uint256(uint160(_address)));
}
}
3 changes: 3 additions & 0 deletions src/Facets/AcrossV4SwapFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,7 @@ contract AcrossV4SwapFacet is

// Validate asset matches for ERC20 flows
bool isNative = LibAsset.isNativeAsset(_bridgeData.sendingAssetId);
// TODO: migrate to LibBytes.toAddress (checked) — see LibBytes v1.1.0
address decodedInputToken = address(
uint160(uint256(params.inputToken))
);
Expand Down Expand Up @@ -609,6 +610,7 @@ contract AcrossV4SwapFacet is
_validateAmount(quote.amount, _bridgeData.minAmount);

// Validate burnToken matches bridgeData asset (native is already excluded by msg.value check above)
// TODO: migrate to LibBytes.toAddress (checked) — see LibBytes v1.1.0
address burnToken = address(uint160(uint256(quote.burnToken)));
if (burnToken != _bridgeData.sendingAssetId) {
revert InformationMismatch();
Expand Down Expand Up @@ -826,6 +828,7 @@ contract AcrossV4SwapFacet is
function _convertAddressToBytes32(
address _address
) internal pure returns (bytes32) {
// TODO: migrate to LibBytes.toBytes32 — see LibBytes v1.1.0
return bytes32(uint256(uint160(_address)));
}
}
3 changes: 3 additions & 0 deletions src/Facets/AllBridgeFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ contract AllBridgeFacet is
} else {
// destination chain is EVM
// make sure that bridgeData and allBridgeData receiver addresses match
// TODO: migrate to LibBytes.toAddress (checked) — see LibBytes v1.1.0
if (
_bridgeData.receiver !=
address(uint160(uint256(_allBridgeData.recipient)))
Expand All @@ -172,6 +173,7 @@ contract AllBridgeFacet is
if (_allBridgeData.payFeeWithSendingAsset) {
// pay fee with sending asset
ALLBRIDGE.swapAndBridge(
// TODO: migrate to LibBytes.toBytes32 — see LibBytes v1.1.0
bytes32(uint256(uint160(_bridgeData.sendingAssetId))),
_bridgeData.minAmount,
_allBridgeData.recipient,
Expand All @@ -184,6 +186,7 @@ contract AllBridgeFacet is
} else {
// pay fee with native asset
ALLBRIDGE.swapAndBridge{ value: _allBridgeData.fees }(
// TODO: migrate to LibBytes.toBytes32 — see LibBytes v1.1.0
bytes32(uint256(uint160(_bridgeData.sendingAssetId))),
_bridgeData.minAmount,
_allBridgeData.recipient,
Expand Down
1 change: 1 addition & 0 deletions src/Facets/CalldataVerificationFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ contract CalldataVerificationFacet {
}

// Convert callToBytes32 to address type and compare them
// TODO: migrate to LibBytes.toAddressUnchecked (intentional truncation) — see LibBytes v1.1.0
address callToAddressFromBytes32 = address(
uint160(uint256(callToBytes32))
);
Expand Down
1 change: 1 addition & 0 deletions src/Facets/CelerCircleBridgeFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ contract CelerCircleBridgeFacet is
CIRCLE_BRIDGE_PROXY.depositForBurn(
_bridgeData.minAmount,
uint64(_bridgeData.destinationChainId),
// TODO: migrate to LibBytes.toBytes32 — see LibBytes v1.1.0
bytes32(uint256(uint160(_bridgeData.receiver))),
USDC,
_celerCircleData.maxFee,
Expand Down
1 change: 1 addition & 0 deletions src/Facets/GlacisFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ contract GlacisFacet is
} else {
// destination chain is EVM
// make sure that bridgeData and glacisData receiver addresses match
// TODO: migrate to LibBytes.toAddress (checked) — see LibBytes v1.1.0
if (
_bridgeData.receiver !=
address(uint160(uint256(_glacisData.receiverAddress)))
Expand Down
1 change: 1 addition & 0 deletions src/Facets/LiFiIntentEscrowFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ contract LiFiIntentEscrowFacet is
} else {
// Check if the receiver is the same according to bridgeData and LIFIIntentData
// Note: We already know 0 <= _bridgeData.receiver < recipient != 0 thus _bridgeData.receiver != 0.
// TODO: migrate to LibBytes.toBytes32 — see LibBytes v1.1.0
if (recipient != bytes32(uint256(uint160(_bridgeData.receiver)))) {
revert InvalidReceiver();
}
Expand Down
1 change: 1 addition & 0 deletions src/Facets/LiFiIntentEscrowFacetV2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ contract LiFiIntentEscrowFacetV2 is
} else {
// Check if the receiver is the same according to bridgeData and LIFIIntentData
// Note: We already know 0 <= _bridgeData.receiver < recipient != 0 thus _bridgeData.receiver != 0.
// TODO: migrate to LibBytes.toBytes32 — see LibBytes v1.1.0
if (recipient != bytes32(uint256(uint160(_bridgeData.receiver)))) {
revert InvalidReceiver();
}
Expand Down
1 change: 1 addition & 0 deletions src/Facets/NEARIntentsFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ contract NEARIntentsFacet is
ILiFi.BridgeData memory _bridgeData,
NEARIntentsData calldata _nearData
) internal view {
// TODO: migrate to LibBytes.toBytes32 — see LibBytes v1.1.0
bytes32 receiverBytes32 = _bridgeData.receiver == NON_EVM_ADDRESS
? _nearData.nonEVMReceiver
: bytes32(uint256(uint160(_bridgeData.receiver)));
Expand Down
1 change: 1 addition & 0 deletions src/Facets/PolymerCCTPFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ contract PolymerCCTPFacet is
TOKEN_MESSENGER.depositForBurn(
bridgeAmount,
_chainIdToDomainId(destinationChainId),
// TODO: migrate to LibBytes.toBytes32 — see LibBytes v1.1.0
bytes32(uint256(uint160(_bridgeData.receiver))),
USDC,
UNRESTRICTED_DESTINATION_CALLER,
Expand Down
1 change: 1 addition & 0 deletions src/Facets/StargateFacetV2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ contract StargateFacetV2 is ILiFi, ReentrancyGuard, SwapperV2, Validatable {
) revert InformationMismatch();

// ensure that receiver addresses match in case of no destination call
// TODO: migrate to LibBytes.toAddress (checked) — see LibBytes v1.1.0
if (
!_bridgeData.hasDestinationCall &&
(_bridgeData.receiver !=
Expand Down
33 changes: 30 additions & 3 deletions src/Libraries/LibBytes.sol
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity ^0.8.17;

/// @custom:version 1.0.0
/// @custom:version 1.1.0
library LibBytes {
// solhint-disable no-inline-assembly

// LibBytes specific errors
error SliceOverflow();
error SliceOutOfBounds();
error AddressOutOfBounds();
error HexLengthInsufficient();
error NotAnAddress(bytes32 value);

bytes16 private constant _SYMBOLS = "0123456789abcdef";

Expand Down Expand Up @@ -121,8 +123,33 @@
buffer[i] = _SYMBOLS[value & 0xf];
value >>= 4;
}
// solhint-disable-next-line gas-custom-errors
require(value == 0, "Strings: hex length insufficient");
if (value != 0) revert HexLengthInsufficient();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toHexString change silently alters SquidFacet bytecode with no version bump.

LibBytes functions are internal pure and get inlined into consumers. SquidFacet.sol:187 is the sole caller of toHexString, so swapping the require-string for revert HexLengthInsufficient() changes SquidFacet's compiled bytecode and its revert data — yet SquidFacet stays at @custom:version 1.0.0 and isn't listed among the "call sites intentionally not migrated". The PR body implies zero facet behavior changes, but this one does change.

Per [CONV:VERSIONING], "new revert on existing function" is a MAJOR-type change, and this is a revert-encoding change on a shipped function. In practice the branch is an unreachable invariant (after the loop value is always 0 for a correctly-sized input), so behavioral/security risk is effectively nil — but it should be surfaced rather than folded silently into a "no facets touched" PR.

Suggestion: either split the toHexString refactor into its own PR acknowledging the SquidFacet impact, or note in the PR body that SquidFacet's bytecode changes and confirm with audit/deploy that an unbumped version is acceptable here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not bumping SquidFacet's version here — but you're right that the transitive bytecode change should be surfaced, so I've added a note to the PR description.

Reasoning: [CONV:VERSIONING] says a library MAJOR bump should be evaluated against consumers, not that any library change bumps them. The toHexString edit isn't a "new revert on existing function" — the revert already existed (require(value == 0, …)); only the revert encoding changed (Error(string)HexLengthInsufficient()). And that branch is unreachable for a correctly-sized length (after the loop value is always 0, and SquidFacet always passes a correct length), so there's zero reachable behavior change at the ABI level → PATCH-class, which doesn't cascade. So SquidFacet stays 1.0.0 and LibBytes' MINOR bump (the three new functions dominate) is correct.

The only real action item was your "surface it rather than fold it silently" point — done: the description now flags that SquidFacet's compiled bytecode/revert-encoding changes (dead-code only) so whoever next deploys it expects the 1.0.0 bytecode to differ.

return string(buffer);
}

/// @notice Left-zero-pads an address into bytes32. Widening — always lossless.
/// @param _addr The address to convert.
/// @return The bytes32 representation of the address.
function toBytes32(address _addr) internal pure returns (bytes32) {
return bytes32(uint256(uint160(_addr)));

Check notice

Code scanning / Olympix Integrated Security

Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast Low

Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
Comment thread
0xDEnYO marked this conversation as resolved.
Dismissed
}

/// @notice Narrows a left-zero-padded bytes32 to an address, reverting if the
/// top 96 bits are set (checked downcast).
/// @param _value The bytes32 value to convert.
/// @return The address representation of the bytes32.
function toAddress(bytes32 _value) internal pure returns (address) {
if (uint256(_value) >> 160 != 0) revert NotAnAddress(_value);
return address(uint160(uint256(_value)));

Check notice

Code scanning / Olympix Integrated Security

Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast Low

Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
Comment thread
0xDEnYO marked this conversation as resolved.
Dismissed
}

/// @notice Narrows a bytes32 to an address by truncation, keeping only the low
/// 160 bits. Use ONLY where dropping the high bits is intentional.
/// @param _value The bytes32 value to convert.
/// @return The address representation of the low 160 bits.
function toAddressUnchecked(
bytes32 _value
) internal pure returns (address) {
return address(uint160(uint256(_value)));

Check notice

Code scanning / Olympix Integrated Security

Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast Low

Performing a narrowing downcast may result in silent overflow due to bit truncation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unsafe-downcast
Comment thread
0xDEnYO marked this conversation as resolved.
Dismissed
}
}
1 change: 1 addition & 0 deletions src/Periphery/ReceiverOIF.sol
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ contract ReceiverOIF is ILiFi, WithdrawablePeriphery, IOutputCallback {
_swapAndCompleteBridgeTokens(
transactionId,
swapData,
// TODO: migrate to LibBytes.toAddress (checked) — see LibBytes v1.1.0
address(uint160(uint256(token))),
payable(receiver),
amount
Expand Down
52 changes: 51 additions & 1 deletion test/solidity/Libraries/LibBytes.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,20 @@ contract LibBytesHarness {
) external pure returns (string memory) {
return LibBytes.toHexString(_value, _length);
}

function toBytes32(address _addr) external pure returns (bytes32) {
return LibBytes.toBytes32(_addr);
}

function toAddress(bytes32 _value) external pure returns (address) {
return LibBytes.toAddress(_value);
}

function toAddressUnchecked(
bytes32 _value
) external pure returns (address) {
return LibBytes.toAddressUnchecked(_value);
}
}

contract LibBytesTest is Test {
Expand Down Expand Up @@ -90,8 +104,44 @@ contract LibBytesTest is Test {
}

function testRevert_toHexString_LengthInsufficient() public {
vm.expectRevert("Strings: hex length insufficient");
vm.expectRevert(LibBytes.HexLengthInsufficient.selector);

harness.toHexString(uint256(0x1234), 1);
}

function test_toBytes32_LeftZeroPadsAddress() public {
address addr = address(0x1234567890AbcdEF1234567890aBcdef12345678);

assertEq(harness.toBytes32(addr), bytes32(uint256(uint160(addr))));
}

function test_toAddress_bytes32_RoundTrips() public {
address addr = address(0x1234567890AbcdEF1234567890aBcdef12345678);

assertEq(harness.toAddress(harness.toBytes32(addr)), addr);
}

function test_toAddress_bytes32_ZeroValue() public {
assertEq(harness.toAddress(bytes32(0)), address(0));
}

function testRevert_toAddress_bytes32_HighBitsSet() public {
bytes32 dirty = bytes32(
(uint256(1) << 160) |
uint160(0x1234567890AbcdEF1234567890aBcdef12345678)
);

vm.expectRevert(
abi.encodeWithSelector(LibBytes.NotAnAddress.selector, dirty)
);

harness.toAddress(dirty);
}

function test_toAddressUnchecked_TruncatesHighBits() public {
address addr = address(0x1234567890AbcdEF1234567890aBcdef12345678);
bytes32 dirty = bytes32((uint256(1) << 160) | uint160(addr));

assertEq(harness.toAddressUnchecked(dirty), addr);
}
}
Loading