Skip to content

Conversation

@nicholaspai
Copy link
Member

@nicholaspai nicholaspai commented Mar 17, 2025

  • The casting to bytes20 will fetch the first 20 bytes of the return value. However, the functions in Solidity return abi.encoded data, which means in particular, that if an address is returned from the feeRecipient function, the returned feeRecipient.length will be equal to 32 and the address will be included in the last 20 bytes of that data (will be padded with 12 zeroes at the beginning). As a result, casting feeRecipient to bytes20 will capture only the first 8 bytes of the returned address. You can get the full address by casting the data to uint160 instead (you may need to cast it to bytes32 and uint256 first).
  • The feeRecipient.length is not validated. It means that for example, in case of a call to nonexistent contract or a call to a contract with a fallback function which doesn't return any data, the code will attempt to cast empty data to bytes20. It seems that casting empty bytes object to bytes20 will return 0, but it would be safer to not rely on that behaviour and validate that the length of feeRecipient object equals 32.
  • Even after fixing the issues above, this check will not be fully reliable as it could for example succeed in case when a TokenMessenger V1 contract had a fallback function returning 32 bytes. This is a very low risk, hence you may keep this logic if you accept the risk, but a fully correct solution would involve introducing an additional parameter to the constructor, which could be then used to determine the CCTP version (this could be an uint256 as it is possible that more CCTP versions will appear in the future). We understand that you wanted to avoid adding this extra parameter (and hence introducing a very large diff), but the most reliable solution would involve introducing it in the constructor.

- The casting to bytes20 will fetch the first 20 bytes of the return value. However, the functions in Solidity return abi.encoded data, which means in particular, that if an address is returned from the feeRecipient function, the returned feeRecipient.length will be equal to 32 and the address will be included in the last 20 bytes of that data (will be padded with 12 zeroes at the beginning). As a result, casting feeRecipient to bytes20 will capture only the first 8 bytes of the returned address. You can get the full address by casting the data to uint160 instead (you may need to cast it to bytes32 and uint256 first).
- The feeRecipient.length is not validated. It means that for example, in case of a call to nonexistent contract or a call to a contract with a fallback function which doesn't return any data, the code will attempt to cast empty data to bytes20. It seems that casting empty bytes object to bytes20 will return 0, but it would be safer to not rely on that behaviour and validate that the length of feeRecipient object equals 32.
- Even after fixing the issues above, this check will not be fully reliable as it could for example succeed in case when a TokenMessenger V1 contract had a fallback function returning 32 bytes. This is a very low risk, hence you may keep this logic if you accept the risk, but a fully correct solution would involve introducing an additional parameter to the constructor, which could be then used to determine the CCTP version (this could be an uint256 as it is possible that more CCTP versions will appear in the future). We understand that you wanted to avoid adding this extra parameter (and hence introducing a very large diff), but the most reliable solution would involve introducing it in the constructor.
@nicholaspai nicholaspai changed the title ix(CCTPAdapter): Fix feeRecipient check to determine V2 vs V1 fix(CCTPAdapter): Fix feeRecipient check to determine V2 vs V1 Mar 17, 2025
@nicholaspai nicholaspai changed the base branch from master to march-25-evm-audit-cctpv2 March 17, 2025 13:45
@nicholaspai nicholaspai marked this pull request as ready for review March 19, 2025 20:49
Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM!

@nicholaspai nicholaspai merged commit c83358d into march-25-evm-audit-cctpv2 Mar 19, 2025
9 checks passed
@nicholaspai nicholaspai deleted the fee-recipient-fix branch March 19, 2025 21:03
nicholaspai added a commit that referenced this pull request Mar 31, 2025
* feat(Linea): Add CCTP V2 support to Linea (#910)

* fix(CCTPAdapter): Fix feeRecipient check to determine V2 vs V1

- The casting to bytes20 will fetch the first 20 bytes of the return value. However, the functions in Solidity return abi.encoded data, which means in particular, that if an address is returned from the feeRecipient function, the returned feeRecipient.length will be equal to 32 and the address will be included in the last 20 bytes of that data (will be padded with 12 zeroes at the beginning). As a result, casting feeRecipient to bytes20 will capture only the first 8 bytes of the returned address. You can get the full address by casting the data to uint160 instead (you may need to cast it to bytes32 and uint256 first).
- The feeRecipient.length is not validated. It means that for example, in case of a call to nonexistent contract or a call to a contract with a fallback function which doesn't return any data, the code will attempt to cast empty data to bytes20. It seems that casting empty bytes object to bytes20 will return 0, but it would be safer to not rely on that behaviour and validate that the length of feeRecipient object equals 32.
- Even after fixing the issues above, this check will not be fully reliable as it could for example succeed in case when a TokenMessenger V1 contract had a fallback function returning 32 bytes. This is a very low risk, hence you may keep this logic if you accept the risk, but a fully correct solution would involve introducing an additional parameter to the constructor, which could be then used to determine the CCTP version (this could be an uint256 as it is possible that more CCTP versions will appear in the future). We understand that you wanted to avoid adding this extra parameter (and hence introducing a very large diff), but the most reliable solution would involve introducing it in the constructor.

* Revert "fix(CCTPAdapter): Fix feeRecipient check to determine V2 vs V1"

This reverts commit cd36e36.

* fix(CCTPAdapter): Fix feeRecipient check to determine V2 vs V1 (#924)

- The casting to bytes20 will fetch the first 20 bytes of the return value. However, the functions in Solidity return abi.encoded data, which means in particular, that if an address is returned from the feeRecipient function, the returned feeRecipient.length will be equal to 32 and the address will be included in the last 20 bytes of that data (will be padded with 12 zeroes at the beginning). As a result, casting feeRecipient to bytes20 will capture only the first 8 bytes of the returned address. You can get the full address by casting the data to uint160 instead (you may need to cast it to bytes32 and uint256 first).
- The feeRecipient.length is not validated. It means that for example, in case of a call to nonexistent contract or a call to a contract with a fallback function which doesn't return any data, the code will attempt to cast empty data to bytes20. It seems that casting empty bytes object to bytes20 will return 0, but it would be safer to not rely on that behaviour and validate that the length of feeRecipient object equals 32.
- Even after fixing the issues above, this check will not be fully reliable as it could for example succeed in case when a TokenMessenger V1 contract had a fallback function returning 32 bytes. This is a very low risk, hence you may keep this logic if you accept the risk, but a fully correct solution would involve introducing an additional parameter to the constructor, which could be then used to determine the CCTP version (this could be an uint256 as it is possible that more CCTP versions will appear in the future). We understand that you wanted to avoid adding this extra parameter (and hence introducing a very large diff), but the most reliable solution would involve introducing it in the constructor.

* improve(CCTPAdapter): Add comment about not casting tokenMinter to V1/V2 (#928)

* improve(cctpAdapter): Fix misleading comments (#929)

* improve(cctpAdapter): typographical errors (#930)

* improve(CCTPAdapter): Remove comment (#931)

* fix(CCTPAdapter): Fix feeRecipient check to determine V2 vs V1

- The casting to bytes20 will fetch the first 20 bytes of the return value. However, the functions in Solidity return abi.encoded data, which means in particular, that if an address is returned from the feeRecipient function, the returned feeRecipient.length will be equal to 32 and the address will be included in the last 20 bytes of that data (will be padded with 12 zeroes at the beginning). As a result, casting feeRecipient to bytes20 will capture only the first 8 bytes of the returned address. You can get the full address by casting the data to uint160 instead (you may need to cast it to bytes32 and uint256 first).
- The feeRecipient.length is not validated. It means that for example, in case of a call to nonexistent contract or a call to a contract with a fallback function which doesn't return any data, the code will attempt to cast empty data to bytes20. It seems that casting empty bytes object to bytes20 will return 0, but it would be safer to not rely on that behaviour and validate that the length of feeRecipient object equals 32.
- Even after fixing the issues above, this check will not be fully reliable as it could for example succeed in case when a TokenMessenger V1 contract had a fallback function returning 32 bytes. This is a very low risk, hence you may keep this logic if you accept the risk, but a fully correct solution would involve introducing an additional parameter to the constructor, which could be then used to determine the CCTP version (this could be an uint256 as it is possible that more CCTP versions will appear in the future). We understand that you wanted to avoid adding this extra parameter (and hence introducing a very large diff), but the most reliable solution would involve introducing it in the constructor.

* Remove comment about cctp domain IDs

* fix(CCTPAdapter): Typographical errors (#935)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants