isContract #1167
isContract #1167
Conversation
WalkthroughA new helper contract and interface are introduced to encapsulate the Changes
Suggested labels
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 GitHub Check: Olympix Integrated Securitysrc/Libraries/LibAsset.sol[notice] 209-209: [failure] 209-209: 🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| address helper | ||
| ) internal view returns (bool) { | ||
| // exactly the same body you have now, but using `helper` instead of EXT_CODE_HELPER | ||
| (bool ok, bytes memory ret) = helper.staticcall( |
Check notice
Code scanning / Olympix Integrated Security
Making an external call without a gas budget may consume all of the transaction's gas, causing it to revert. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/call-without-gas-budget Low
| address helper | ||
| ) internal view returns (bool) { | ||
| // exactly the same body you have now, but using `helper` instead of EXT_CODE_HELPER | ||
| (bool ok, bytes memory ret) = helper.staticcall( |
Check failure
Code scanning / Olympix Integrated Security
The contract uses low-level calls without properly verifying the input parameters, potentially leading to unexpected behavior or vulnerabilities. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/low-level-call-params-verified Critical
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Helpers/ExtCodeHelper.sol(1 hunks)src/Interfaces/IExtcodeHelper.sol(1 hunks)src/Libraries/LibAsset.sol(3 hunks)src/Libraries/LibSwap.sol(2 hunks)test/solidity/Libraries/LibAsset.t.sol(3 hunks)
🧰 Additional context used
🪛 GitHub Check: Olympix Integrated Security
src/Libraries/LibSwap.sol
[notice] 12-12:
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
src/Libraries/LibAsset.sol
[notice] 213-213:
Making an external call without a gas budget may consume all of the transaction's gas, causing it to revert. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/call-without-gas-budget
[failure] 213-213:
The contract uses low-level calls without properly verifying the input parameters, potentially leading to unexpected behavior or vulnerabilities. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/low-level-call-params-verified
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (10)
src/Interfaces/IExtcodeHelper.sol (1)
1-10: LGTM! Clean interface design.The interface is well-documented and follows Solidity conventions. The function signature correctly defines the expected return values for delegation info extraction.
src/Libraries/LibSwap.sol (1)
35-36: Good integration of enhanced contract detection.The migration from
isContracttoisContractWithHelperproperly leverages the new EIP-7702 aware validation.src/Libraries/LibAsset.sol (3)
1-1: License update acknowledged.Changed from UNLICENSED to MIT license.
208-237: Well-implemented contract detection with EIP-7702 support.The function correctly:
- Uses staticcall for gas efficiency and safety
- Validates return data length before decoding
- Falls back to standard extcodesize check on failure
- Handles delegated contracts via the DELEGATION_DESIGNATOR prefix
The static analysis warnings are false positives:
- Gas budget warning: Not applicable to view functions
- Parameter verification: Parameters are properly encoded with abi.encodeWithSelector
🧰 Tools
🪛 GitHub Check: Olympix Integrated Security
[notice] 213-213:
Making an external call without a gas budget may consume all of the transaction's gas, causing it to revert. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/call-without-gas-budget
[failure] 213-213:
The contract uses low-level calls without properly verifying the input parameters, potentially leading to unexpected behavior or vulnerabilities. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/low-level-call-params-verified
239-245: Good practice: Preserving original function for backward compatibility.Keeping the original
isContractfunction ensures existing code continues to work while new code can adopt the enhancedisContractWithHelperfunctionality.test/solidity/Libraries/LibAsset.t.sol (5)
11-19: Good test helper implementation for simulating failure scenarios.The
RevertingHelpercontract effectively simulates the behavior when EXTCODECOPY is not supported, allowing thorough testing of the fallback mechanism.
22-26: Well-structured dependency injection for the helper contract.The modifications properly implement dependency injection for the ExtcodeHelper, making the contract more testable and flexible.
Also applies to: 61-63
68-72: Correct test setup with proper deployment order.The ExtcodeHelper is correctly deployed and injected into the LibAssetImplementer.
198-208: Good test contract design for flexible helper testing.The generic helper address allows testing with different IExtcodeHelper implementations, particularly the RevertingHelper.
210-252: Comprehensive fallback mechanism testing.Excellent test coverage for the fallback behavior when the ExtcodeHelper fails. The tests effectively verify that:
- Contract detection falls back to extcodesize for regular contracts
- EOAs are correctly identified when the helper reverts
- The delegation code path is bypassed when the helper fails
The test names and assertions clearly communicate the expected behavior.
| assembly { | ||
| // buf layout at buf+0x20: [ prefix:3 | delegate:20 ] | ||
| let ptr := add(buf, 0x20) | ||
| prefix := mload(ptr) // loads first 32 bytes, high-order 3 bytes are our prefix | ||
| delegate := shr(96, mload(add(ptr, 3))) // loads next 32 bytes, shift right 96 bits to drop padding | ||
| } |
There was a problem hiding this comment.
Fix unsafe memory access when extracting delegate address.
The current implementation reads past the buffer boundary. When mload(add(ptr, 3)) is called, it reads 32 bytes starting from offset 3, but the buffer only contains 23 bytes. This reads 12 bytes of potentially uninitialized memory.
Apply this safer alternative:
assembly {
// buf layout at buf+0x20: [ prefix:3 | delegate:20 ]
let ptr := add(buf, 0x20)
- prefix := mload(ptr) // loads first 32 bytes, high-order 3 bytes are our prefix
- delegate := shr(96, mload(add(ptr, 3))) // loads next 32 bytes, shift right 96 bits to drop padding
+ let data := mload(ptr) // loads 32 bytes safely (23 bytes data + 9 bytes zero padding)
+ prefix := data // automatically takes first 3 bytes when casting to bytes3
+ // Extract delegate by removing 3-byte prefix and aligning to address
+ delegate := shr(96, shl(24, data))
}Alternatively, use masking to ensure clean extraction:
delegate := and(shr(24, data), 0xffffffffffffffffffffffffffffffffffffffff)🤖 Prompt for AI Agents
In src/Helpers/ExtCodeHelper.sol between lines 14 and 19, the assembly code
reads 32 bytes starting at an offset that causes it to access memory beyond the
buffer boundary, leading to unsafe memory access. To fix this, avoid loading
memory starting at an unaligned offset; instead, load the 32 bytes starting at
the buffer pointer and then use bit shifting and masking to extract the delegate
address safely within the buffer limits. Replace the current mload and shr
operations with a single mload followed by a shift right by 24 bits and an and
mask with 0xffffffffffffffffffffffffffffffffffffffff to cleanly extract the
20-byte delegate address without reading out-of-bounds memory.
src/Libraries/LibSwap.sol
Outdated
| address internal constant EXT_CODE_HELPER = | ||
| address(uint160(uint256(keccak256("EXT_CODE_HELPER")))); |
There was a problem hiding this comment.
Critical: Helper address inconsistency between libraries.
This computes the helper address as keccak256("EXT_CODE_HELPER"), but LibAsset.sol uses a fixed address 0x7000000000000000000000000000000000000000. These will reference different contracts, causing the validation to fail.
Align with LibAsset's approach:
-address internal constant EXT_CODE_HELPER =
- address(uint160(uint256(keccak256("EXT_CODE_HELPER"))));
+address internal constant EXT_CODE_HELPER =
+ 0x7000000000000000000000000000000000000000;Note: The static analysis warning about narrowing downcast is a false positive - casting uint256 to uint160 for address creation is standard practice.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| address internal constant EXT_CODE_HELPER = | |
| address(uint160(uint256(keccak256("EXT_CODE_HELPER")))); | |
| address internal constant EXT_CODE_HELPER = | |
| 0x7000000000000000000000000000000000000000; |
🧰 Tools
🪛 GitHub Check: Olympix Integrated Security
[notice] 12-12:
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
🤖 Prompt for AI Agents
In src/Libraries/LibSwap.sol at lines 11-12, the helper address is computed
using keccak256("EXT_CODE_HELPER"), which differs from the fixed address used in
LibAsset.sol (0x7000000000000000000000000000000000000000). To fix this, replace
the computed address with the fixed address literal used in LibAsset.sol to
ensure both libraries reference the same helper contract and avoid validation
failures.
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)