improve(cctpAdapter): typographical errors #930
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mrice32
approved these changes
Mar 19, 2025
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.