Skip to content

Reject interoperable addresses whith both chain reference and addresses empty #6331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Amxx merged 4 commits into from
Feb 12, 2026
+18 −7
Merged

Reject interoperable addresses whith both chain reference and addresses empty #6331

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd8405d
Reject interoperable addresses whith both chain reference and address…
Amxx Feb 6, 2026
b1b65be
Apply suggestion from @Amxx
Amxx Feb 9, 2026
5444015
Merge branch 'master' into fix/erc7930/reject-empty-reference-and-add…
Amxx Feb 10, 2026
0e591ba
Apply suggestion from @Amxx
Amxx Feb 10, 2026
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 .changeset/smooth-cows-notice.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`InteroperableAddress`: reject inputs with both chain reference and addresses empty.
12 changes: 8 additions & 4 deletions contracts/utils/draft-InteroperableAddress.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,10 @@ library InteroperableAddress {
bytes memory self
) internal pure returns (bool success, bytes2 chainType, bytes memory chainReference, bytes memory addr) {
unchecked {
success = true;
if (self.length < 0x06) return (false, 0x0000, _emptyBytesMemory(), _emptyBytesMemory());

bytes2 version = _readBytes2(self, 0x00);
if (version != bytes2(0x0001)) return (false, 0x0000, _emptyBytesMemory(), _emptyBytesMemory());
chainType = _readBytes2(self, 0x02);

uint8 chainReferenceLength = uint8(self[0x04]);
if (self.length < 0x06 + chainReferenceLength)
Expand All @@ -112,6 +110,10 @@ library InteroperableAddress {
if (self.length < 0x06 + chainReferenceLength + addrLength)
return (false, 0x0000, _emptyBytesMemory(), _emptyBytesMemory());
addr = self.slice(0x06 + chainReferenceLength, 0x06 + chainReferenceLength + addrLength);

// At least one of chainReference or addr must be non-empty
success = (chainReferenceLength > 0) || (addrLength > 0);
chainType = success ? _readBytes2(self, 0x02) : bytes2(0);
}
}

Expand All @@ -122,12 +124,10 @@ library InteroperableAddress {
bytes calldata self
) internal pure returns (bool success, bytes2 chainType, bytes calldata chainReference, bytes calldata addr) {
unchecked {
success = true;
if (self.length < 0x06) return (false, 0x0000, Calldata.emptyBytes(), Calldata.emptyBytes());

bytes2 version = _readBytes2Calldata(self, 0x00);
if (version != bytes2(0x0001)) return (false, 0x0000, Calldata.emptyBytes(), Calldata.emptyBytes());
chainType = _readBytes2Calldata(self, 0x02);

uint8 chainReferenceLength = uint8(self[0x04]);
if (self.length < 0x06 + chainReferenceLength)
Expand All @@ -138,6 +138,10 @@ library InteroperableAddress {
if (self.length < 0x06 + chainReferenceLength + addrLength)
return (false, 0x0000, Calldata.emptyBytes(), Calldata.emptyBytes());
addr = self[0x06 + chainReferenceLength:0x06 + chainReferenceLength + addrLength];

// At least one of chainReference or addr must be non-empty
success = (chainReferenceLength > 0) || (addrLength > 0);
chainType = success ? _readBytes2Calldata(self, 0x02) : bytes2(0);
}
}

Expand Down
8 changes: 5 additions & 3 deletions test/utils/draft-InteroperableAddress.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,13 @@ describe('ERC7390', function () {
// version 2 + some data
'unsupported version': '0x00020000010100',
// version + ref: missing chainReferenceLength and addressLength
Copy link
Member

@ernestognw ernestognw Feb 10, 2026

Choose a reason for hiding this comment

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

Suggested change
// version + ref: missing chainReferenceLength and addressLength
// version + ref: missing chainReferenceLength and address buffer shorter than its declared length

Copy link
Collaborator Author

@Amxx Amxx Feb 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

In this case there is not address length.

0x00010042....
  ^^^^         version
      ^^^^     chain type
          ^^   expected location of the chainReferenceLength
            ^^ expected location of the addressLength (if the chainReferenceLength was zero)

There is no chainReferenceLength, and no addressLength. We are not even looking into the chain reference and address buffer

'too short (case 1)': '0x00010000',
'too short (case 1)': '0x00010042',
Copy link
Member

@ernestognw ernestognw Feb 10, 2026

Choose a reason for hiding this comment

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

I don't get why we're changing this test case. With the new code, this is rejected because both chainReferenceLength and addrLength are zero. So the old test still passes: parseV1 reverts and tryParseV1 returns false.

Copy link
Collaborator Author

@Amxx Amxx Feb 11, 2026

Choose a reason for hiding this comment

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

The current test uses an (invalid) interroperable address with a chainType = 0. When we look at the output of the tryParse, and we see success = false, chainType = 0, we have no way to know if the chain type was parsed (and contains a zero) or if it was not parsed, because the error was detected.

That is why I think we should use a non zero chaintype to make a clear distinction.

// version + ref + chainReference: missing addressLength
'too short (case 2)': '0x000100000101',
'too short (case 2)': '0x000100420101',
Copy link
Member

@ernestognw ernestognw Feb 10, 2026

Choose a reason for hiding this comment

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

Also the old test here was fine

Copy link
Collaborator Author

@Amxx Amxx Feb 11, 2026

Choose a reason for hiding this comment

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

same answer :)

// version + ref + chainReference + addressLength + part of the address: missing 2 bytes of the address
'too short (case 3)': '0x00010000010114d8da6bf26964af9d7eed9e03e53415d37aa9',
'too short (case 3)': '0x00010042010114d8da6bf26964af9d7eed9e03e53415d37aa9',
Copy link
Member

@ernestognw ernestognw Feb 10, 2026

Choose a reason for hiding this comment

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

Same

Copy link
Collaborator Author

@Amxx Amxx Feb 11, 2026

Choose a reason for hiding this comment

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

same answer :)

// empry chain reference and address
'empty chain reference and address': '0x000100420000',
})) {
it(title, async function () {
await expect(this.mock.$parseV1(binary))
Expand Down
Loading