HUBS-224 crowdsale with individual locking staking periods #170
Conversation
adds condition to faile when duration < 7 days (enfoced by token vesting contract) replaces bakedin locking contract with an external deployment for verifiability Signed-off-by: Stefan Adolf <[email protected]>
adds staking duration parameters to all staking tests drops old V23 sale rollout script Signed-off-by: Stefan Adolf <[email protected]>
WalkthroughThis pull request introduces significant enhancements to the contract infrastructure, focusing on the implementation of a new Changes
Sequence DiagramsequenceDiagram
participant Deployer
participant TimelockedToken
participant StakedLockingCrowdSale
participant LockingCrowdSale
Deployer->>TimelockedToken: Create Implementation
Deployer->>StakedLockingCrowdSale: Deploy with TimelockedToken
Deployer->>LockingCrowdSale: Deploy with TimelockedToken
Deployer->>StakedLockingCrowdSale: Start Sale
StakedLockingCrowdSale->>TimelockedToken: Create Locking Contract
StakedLockingCrowdSale->>StakedLockingCrowdSale: Store Stake Vesting Duration
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Signed-off-by: Stefan Adolf <[email protected]>
elmariachi111
force-pushed
the
feature/hubs-224-crowdsale-with-individual-locking-staking-periods
branch
from
def2bed to
8d83cfc
Compare
Signed-off-by: Stefan Adolf <[email protected]>
Signed-off-by: Stefan Adolf <[email protected]>
Signed-off-by: Stefan Adolf <[email protected]>
Signed-off-by: Stefan Adolf <[email protected]>
Signed-off-by: Stefan Adolf <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (18)
subgraph/src/lockingCrowdSaleMapping.ts (3)
2-2: Consider renaming the imported BigInt.
Although this is a type from '@graphprotocol/graph-ts', it shadows the global 'BigInt' in TypeScript, which might lead to confusion. Renaming avoids collisions and clarifies the origin.-import { - BigInt, - ... -} from '@graphprotocol/graph-ts' +import { + BigInt as GraphBigInt, + ... +} from '@graphprotocol/graph-ts'🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
67-80: Ensure locked token uniqueness is correct and tested.
The code checks if the new locking token differs from the existing one and logs an error if they don't match. Check if any fallback or handle path is needed when a mismatch occurs, especially if an IP might inadvertently reference multiple locking tokens.
81-87: Optional: Distinguish the crowdSale.type assignment for future expansions.
Currently, "STAKED_LOCKING_CROWDSALE" is assigned. If there’s a possibility of different crowd sale subtypes, consider storing them in a more flexible, future-proof manner (e.g. an enum or additional fields).src/crowdsale/LockingCrowdSale.sol (1)
65-70: Consider clarifying the fallback logic with a comment.
When the trustee returns no existing contract, a fresh TimelockedToken is created. A short comment explaining the fallback scenario can improve clarity for future maintainers.src/crowdsale/StakedLockingCrowdSale.sol (1)
127-130: Large vesting durations.
You enforce a minimum of 7 days. Consider also an upper limit if business logic demands a max..env.example (2)
29-29: Standardize comment formattingThe comment style is inconsistent. Consider using a consistent format:
-#iptoken implementation=0x0DCd1Bf9A1b36cE34237eEaFef220932846BCD82 -#timelocked token implementation=0x0B306BF915C4d645ff596e518fAf3F9669b97016 +# IPToken implementation = 0x0DCd1Bf9A1b36cE34237eEaFef220932846BCD82 +# Timelocked token implementation = 0x0B306BF915C4d645ff596e518fAf3F9669b97016Also applies to: 31-31
🧰 Tools
🪛 Gitleaks (8.21.2)
29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
34-36: Consider grouping related addressesConsider grouping the LOCKED_IPTS_ADDRESS with other IPT-related addresses for better organization and readability.
USDC6_ADDRESS=0x3Aa5ebB10DC797CAC828524e59A333d0A371443c WETH_ADDRESS=0x322813Fd9A801c5507c9de605d63CEA4f2CE6c44 PLAIN_CROWDSALE_ADDRESS=0x09635F643e140090A9A8Dcd712eD6285858ceBef -#these are generated when running the fixture scripts -IPTS_ADDRESS=0x8dAF17A20c9DBA35f005b6324F493785D239719d -LOCKED_IPTS_ADDRESS=0x24B3c7704709ed1491473F30393FFc93cFB0FC34 +# IPT-related addresses (generated when running the fixture scripts) +IPTS_ADDRESS=0x8dAF17A20c9DBA35f005b6324F493785D239719d +LOCKED_IPTS_ADDRESS=0x24B3c7704709ed1491473F30393FFc93cFB0FC34Also applies to: 40-40
setupLocal.sh (2)
Line range hint
21-32: Consider adding validation for option combinationsThe script might benefit from validating mutually exclusive options or requiring certain option combinations.
while getopts "fx" opt; do case ${opt} in f) fixtures=true ;; x) + if [ "$fixtures" = false ]; then + echo "Error: -x requires -f option" + exit 1 + fi extrafixtures=true ;; *) echo "Unknown option: $1" exit 1 ;; esac done
Line range hint
66-73: Consider parameterizing the sleep durationThe hardcoded sleep duration of 15/16 seconds might need adjustment based on network conditions. Consider making it configurable.
+# Default sleep duration between operations (in seconds) +SLEEP_DURATION=${SLEEP_DURATION:-15} + if $extrafixtures; then echo "Running extra fixture scripts (crowdsales)." $FSC script/dev/CrowdSale.s.sol:FixtureCrowdSale - echo "Waiting 15 seconds until claiming plain sale..." - sleep 16 + echo "Waiting ${SLEEP_DURATION} seconds until claiming plain sale..." + sleep $((SLEEP_DURATION + 1))script/DeployTokenizer.s.sol (2)
31-33: Improve comment clarity and consider adding eventsThe comment about contract verification could be more descriptive. Also, consider emitting events for important state changes.
- //this allows the default TimelockedToken implementation to be verified on chain explorers + // Deploy TimelockedToken implementation first to enable contract verification on block explorers TimelockedToken timelockedTokenImplementation = new TimelockedToken(); + emit TimelockedTokenImplementationDeployed(address(timelockedTokenImplementation)); StakedLockingCrowdSale stakedLockingCrowdSale = new StakedLockingCrowdSale(timelockedTokenImplementation); + emit StakedLockingCrowdSaleDeployed(address(stakedLockingCrowdSale));Don't forget to add the event declarations at the contract level:
event TimelockedTokenImplementationDeployed(address implementation); event StakedLockingCrowdSaleDeployed(address crowdSale);
40-41: Consider standardizing the console log formatThe console log format for the timelocked token implementation differs from other logs. Consider standardizing it:
- console.log("timelocked token implementation=%s", address(timelockedTokenImplementation)); + console.log("TIMELOCKED_TOKEN_IMPLEMENTATION_ADDRESS=%s", address(timelockedTokenImplementation));script/prod/RolloutV25Sale.sol (2)
15-15: Document multisig addressesThe multisig addresses are hardcoded without clear documentation of their purpose and ownership. Consider adding comments explaining:
- Who controls these multisigs
- Why different addresses are used for mainnet vs other networks
- Whether these have been verified as the correct addresses
Also applies to: 34-34
18-21: Consider emitting events for important state changesImportant contract deployments and ownership transfers should emit events for better traceability. Consider:
- Adding events to the contracts being deployed
- Using OpenZeppelin's
Initializableto prevent double initializationExample implementation:
event CrowdsaleDeployed(address indexed implementation, address indexed proxy, address indexed owner);Also applies to: 40-43
test/CrowdSaleWithNonStandardERC20Test.t.sol (1)
87-87: Verify consistent vesting duration usageThe startSale function calls now include the vesting duration parameter. The values used are:
- 60 days for standard sales
- 7 days for the 2 decimals test case
Consider extracting these durations as constants to maintain consistency and make updates easier.
+ uint256 constant STANDARD_VESTING_DURATION = 60 days; + uint256 constant SHORT_VESTING_DURATION = 7 days;Also applies to: 125-125, 183-183, 245-245, 310-310
test/CrowdSaleLockedStakedTest.t.sol (1)
459-461: Implement the pending test for locking contract reuseThe TODO comment indicates a missing test case for demonstrating how locking contracts are reused.
Would you like me to help implement this test case or create a GitHub issue to track this task?
subgraph/abis/LockingCrowdSale.json (1)
319-435: Consider adding validation for minimum sale durationThe
startSalefunctions should consider adding validation for minimum sale duration to prevent flash sales that could be manipulated.Consider adding a minimum duration check in the implementation to prevent potential market manipulation through extremely short sales.
README.md (2)
53-54: Consider improving the format of implementation addressesThe timelocked token implementation address and the deprecated crowdsale note could be better formatted for consistency with the rest of the document. Consider using the table format or a dedicated section for implementation addresses.
-timelocked token implementation=0xF8F79c1E02387b0Fc9DE0945cD9A2c06F127D851 -~~new staked crowdsale (not used yet HUBS-224) 0x7eeb7113f90893fb95c6666e3930235850f2bc6A)~~ +| Contract | Address | Status | +| -------- | ------- | ------ | +| Timelocked Token Implementation | 0xF8F79c1E02387b0Fc9DE0945cD9A2c06F127D851 | Active | +| Staked Crowdsale | 0x7eeb7113f90893fb95c6666e3930235850f2bc6A | Deprecated (HUBS-224) |🧰 Tools
🪛 Gitleaks (8.21.2)
53-53: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
212-214: Fix typo in verification instructionsThe word "crowdsale" is repeated in the instruction header.
-Verifying a the staked crowdsale crowdsale +Verifying the staked crowdsale🧰 Tools
🪛 LanguageTool
[grammar] ~212-~212: Two determiners in a row. Choose either “a” or “the”.
Context: ....sol:StakedLockingCrowdSaleVerifying a the staked crowdsale crowdsaleforge verif...(DT_DT)
[duplication] ~212-~212: Possible typo: you repeated a word
Context: ...ckingCrowdSaleVerifying a the staked crowdsale crowdsaleforge verify-contract --chain-id=11155...(ENGLISH_WORD_REPEAT_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.env.example(1 hunks)README.md(2 hunks)script/DeployTokenizer.s.sol(2 hunks)script/dev/CrowdSale.s.sol(2 hunks)script/dev/SignTermsMessage.s.sol(1 hunks)script/dev/Tokenizer.s.sol(1 hunks)script/prod/RolloutV23Sale.sol(0 hunks)script/prod/RolloutV25Sale.sol(1 hunks)setupLocal.sh(1 hunks)src/crowdsale/LockingCrowdSale.sol(3 hunks)src/crowdsale/StakedLockingCrowdSale.sol(7 hunks)subgraph/abis/IPNFT.json(1 hunks)subgraph/abis/LockingCrowdSale.json(1 hunks)subgraph/abis/StakedLockingCrowdSale.json(5 hunks)subgraph/makeAbis.sh(1 hunks)subgraph/networks.json(3 hunks)subgraph/schema.graphql(1 hunks)subgraph/src/lockingCrowdSaleMapping.ts(1 hunks)subgraph/subgraph.yaml(2 hunks)test/CrowdSaleLockedStakedTest.t.sol(9 hunks)test/CrowdSaleLockedTest.t.sol(1 hunks)test/CrowdSalePermissioned.t.sol(2 hunks)test/CrowdSaleWithNonStandardERC20Test.t.sol(6 hunks)
💤 Files with no reviewable changes (1)
- script/prod/RolloutV23Sale.sol
✅ Files skipped from review due to trivial changes (1)
- script/dev/Tokenizer.s.sol
🧰 Additional context used
🪛 Gitleaks (8.21.2)
README.md
53-53: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
.env.example
29-29: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
31-31: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
README.md
[grammar] ~212-~212: Two determiners in a row. Choose either “a” or “the”.
Context: ....sol:StakedLockingCrowdSale Verifying a the staked crowdsale crowdsaleforge verif...
(DT_DT)
[duplication] ~212-~212: Possible typo: you repeated a word
Context: ...ckingCrowdSale Verifying a the staked crowdsale crowdsaleforge verify-contract --chain-id=11155...
(ENGLISH_WORD_REPEAT_RULE)
🪛 Biome (1.9.4)
subgraph/src/lockingCrowdSaleMapping.ts
[error] 2-2: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (34)
subgraph/src/lockingCrowdSaleMapping.ts (1)
32-87: Verify error handling when loading the CrowdSale/ IPT entities.
The logic already logs an error if the entity doesn't exist, then returns. Confirm that upstream code or tests ensure that the missing entities do not leave the subgraph in an unexpected state.
✅ Verification successful
Let me check the error handling patterns in these files to ensure consistency.
Error handling for CrowdSale/IPT entities is consistent and safe
The error handling patterns across the codebase are consistent and properly handle missing entities:
- All CrowdSale.load() calls check for null and log appropriate errors before returning
- All IPT.load() calls (except in common.ts which has a valid different use case) check for null, log errors, and return
- The error handling in lockingCrowdSaleMapping.ts follows the same pattern as other files
- Early returns prevent any further processing that could leave the subgraph in an inconsistent state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Use ast-grep to locate entity loading in the subgraph code to ensure consistent error handling
ast-grep --pattern $'CrowdSale.load($_)'
ast-grep --pattern $'IPT.load($_)'
Length of output: 1608
Script:
#!/bin/bash
# Check error handling patterns in files that load CrowdSale and IPT entities
rg "CrowdSale.load|IPT.load" -A 5 subgraph/src/
Length of output: 6617
src/crowdsale/LockingCrowdSale.sol (6)
15-17: Contract naming clarity.
The interface name, ITrustedLockingContracts, accurately conveys that it provides externally trusted references to TimelockedToken instances. This is straightforward, good naming.
41-43: Constructor injection approach looks solid.
Injecting the TimelockedToken implementation address via the constructor is a robust approach that aligns well with proxy-based patterns.
50-52: Check the usage scope of trustLockingContractSource.
This function sets a trustee for locking contracts. For clarity and security, confirm that only the owner may call it and that calling it multiple times is safe.
Line range hint 84-88: Locking duration upper bound check is valid and conventional.
The revert condition for > 366 days is explicit and straightforward, preventing unreasonably long locks.
138-138: Clone usage is correct; confirm initialization is safe from reentrancy.
The code clones and initializes a TimelockedToken. Consider verifying that reentrancy cannot occur during initialization.
24-35: Consider verifying correctness of public variables.
The new public mappings and references allow external reading of lockingContracts. Confirm that read access to these mappings does not expose sensitive data or compromise security.
✅ Verification successful
Based on the search results, I can now generate the final response:
Public mappings for locking contracts are safely implemented
The lockingContracts mapping and lockingContractTrustee are used appropriately with proper access controls:
- Locking contracts can only be created through
createOrReturnTimelockContract()which enforces proper initialization - Writing to
lockingContractsonly happens in controlled contexts with owner-only access for trustee configuration - The mappings only expose read access to contract addresses, which is necessary for verification and integration
- Test coverage confirms expected behavior for token locking, vesting and release mechanics
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for usage of lockingContracts and lockingContractTrustee references to ensure no unintended writes or missed permission checks
rg lockingContracts -A 10
rg lockingContractTrustee -A 10
Length of output: 31917
src/crowdsale/StakedLockingCrowdSale.sol (4)
41-42: Mapping for staking durations is aligned with the new schema.
The new mapping 'salesStakeVestingDuration' is consistent with the broader locking logic.
57-58: Constructor chaining is appropriate.
Passing the _timelockedTokenImplementation to LockingCrowdSale ensures consistent TimelockedToken usage across both contracts.
88-90: Ensure lockingDuration and stakesVestingDuration validations remain consistent.
Both parameters have separate checks (e.g., stake vesting must be ≥ 7 days, while locking must be ≤ 366 days). Confirm that test cases cover boundary scenarios (e.g. exactly 7 days, exactly 366 days).
Also applies to: 98-99
Line range hint 196-222: Double-check vesting logic in the claim function.
Tokens are immediately transferred to the user if the sale is closed + vesting duration is exceeded. Otherwise, they’re locked in the vesting contract. This logic seems correct; ensure thorough tests for boundary conditions.
script/dev/SignTermsMessage.s.sol (1)
9-9: Marking 'run' as 'view' is sensible.
The function only logs a signature and does not mutate state. This change aligns with best practices for read-only logic.
setupLocal.sh (1)
12-18: LGTM! Well-documented help function
The help function clearly describes the available options and their purposes.
script/DeployTokenizer.s.sol (1)
14-14: Verify the initialization sequence
The TimelockedToken implementation is crucial for the StakedLockingCrowdSale. Let's verify that all contracts using this implementation are properly initialized.
Also applies to: 31-33
✅ Verification successful
Based on the shell script results, I can now generate the final response:
TimelockedToken initialization sequence verified and properly implemented
The TimelockedToken implementation is correctly initialized across the codebase:
- All instances follow the same pattern: create TimelockedToken implementation first, then pass it to StakedLockingCrowdSale constructor
- Proper initialization sequence is maintained in both test and deployment scripts
- Integration with TokenVesting is handled correctly where required
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the initialization sequence and usage of TimelockedToken
# Check for TimelockedToken usage in other contracts
echo "Checking TimelockedToken usage:"
ast-grep --pattern 'contract $_ {
$$$
TimelockedToken $_
$$$
}'
# Check for proper initialization patterns
echo "Checking initialization patterns:"
rg -A 5 "TimelockedToken.*new"
Length of output: 4164
subgraph/schema.graphql (1)
123-123: LGTM! Schema change aligns with locking crowdsale implementation
The addition of LOCKING_CROWDSALE to the SaleType enum properly integrates with the existing schema structure, particularly with the TimelockedToken and LockedSchedule entities that handle token locking periods.
subgraph/networks.json (1)
19-22: Verify sepolia deployment
The sepolia address appears to be a real deployment. Let's verify its bytecode matches our contract.
subgraph/makeAbis.sh (1)
12-12: LGTM: ABI extraction for LockingCrowdSale
The addition of LockingCrowdSale ABI extraction aligns with the existing pattern and is correctly implemented.
test/CrowdSalePermissioned.t.sol (2)
69-70: LGTM: TimelockedToken integration
Proper initialization of TimelockedToken and its integration with StakedLockingCrowdSale.
107-107: Verify staking and locking durations
The test uses equal durations (60 days) for both staking and locking periods. Consider adding test cases with different durations to ensure the contract handles varying periods correctly.
script/dev/CrowdSale.s.sol (3)
38-40: LGTM: Deployment configuration
Proper initialization and deployment of TimelockedToken and StakedLockingCrowdSale contracts.
46-47: LGTM: Enhanced logging
Good addition of logging statements for contract addresses, which aids in deployment verification.
155-156: Consider standardizing durations across tests and fixtures
The fixture uses 7-day periods while the test uses 60-day periods. Consider standardizing these durations or documenting the reason for the difference.
subgraph/subgraph.yaml (2)
142-159: LGTM: Event handlers are properly configured
The LockingCrowdSale contract's event handlers are correctly configured to handle all relevant events including Started, Settled, Failed, Bid, LockingContractCreated, and various claim events.
127-127: Verify deployment address configuration
The LockingCrowdSale contract address appears to be using a placeholder value (0xbaadf00dbaadf00dbaadf00dbaadf00dbaadf00d). This needs to be updated with the actual deployed contract address.
test/CrowdSaleLockedTest.t.sol (1)
28-29: LGTM: Proper initialization of TimelockedToken
The TimelockedToken implementation is correctly instantiated and passed to the LockingCrowdSale constructor, following the new contract architecture.
test/CrowdSaleWithNonStandardERC20Test.t.sol (1)
53-54: LGTM: Consistent TimelockedToken implementation
The TimelockedToken setup matches the pattern used in other test files, maintaining consistency across the test suite.
subgraph/abis/IPNFT.json (1)
190-208: Document the purpose and behavior of isPoi function
The newly added isPoi function lacks documentation explaining its purpose and behavior. Since it's marked as pure, it suggests the result is deterministic based solely on the tokenId parameter, but the logic for this determination is unclear.
Let's check for any related documentation or usage:
test/CrowdSaleLockedStakedTest.t.sol (2)
58-60: LGTM: Proper initialization of TimelockedToken
The TimelockedToken implementation is correctly initialized and passed to the StakedLockingCrowdSale constructor.
400-404: LGTM: Proper validation of duration parameters
The test correctly verifies that the staking duration must be less than the vesting duration, ensuring the contract's invariants are maintained.
subgraph/abis/LockingCrowdSale.json (3)
1-12: LGTM: Constructor properly accepts TimelockedToken implementation
The constructor is well-defined with proper input validation through the contract type specification.
463-486: LGTM: Well-structured events with proper indexing
The events are well-designed with:
- Proper indexing of key parameters for efficient filtering
- Comprehensive event data for tracking important state changes
- Clear separation of concerns between different event types
Also applies to: 487-517, 518-543, 544-556, 557-569, 570-588, 589-607, 608-632, 633-780
781-862: LGTM: Comprehensive error handling
The contract includes a complete set of custom errors that:
- Provide specific error cases for better error handling
- Include relevant error parameters where needed
- Use descriptive names that clearly indicate the error condition
subgraph/abis/StakedLockingCrowdSale.json (2)
2-12: LGTM: Constructor properly initializes timelocked token implementation
The constructor correctly accepts and stores the timelocked token implementation address, which is essential for the contract's functionality.
322-340: Verify vesting duration constraints
The addition of salesStakeVestingDuration and stakesVestingDuration parameters requires validation:
- Ensure the vesting duration cannot be set to zero
- Verify the vesting duration cannot exceed reasonable limits
- Consider adding checks for the relationship between locking and vesting durations
Also applies to: 485-489
✅ Verification successful
Vesting duration constraints are properly implemented
The code shows that vesting duration constraints are properly handled:
- Zero duration check:
if (stakesVestingDuration < 7 days) { revert InvalidDuration(); } - Reasonable limits: Minimum of 7 days is enforced as required by TokenVesting contract
- Relationship with locking duration: The implementation correctly handles both durations independently:
- Locking duration for auction tokens is handled by parent contract
- Vesting duration for stakes is enforced to be at least 7 days
- If vesting period has expired, tokens are transferred directly instead of being vested
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for duration validation checks
ast-grep --pattern 'function startSale($$$) {
$$$
require($duration > 0, $_)
$$$
}'
# Look for maximum duration constraints
ast-grep --pattern 'function startSale($$$) {
$$$
require($duration <= $_maximum, $_)
$$$
}'
Length of output: 192
Script:
#!/bin/bash
# Search for Solidity files containing StakedLockingCrowdSale contract
fd -e sol | xargs rg -l "contract StakedLockingCrowdSale"
# Search for duration validation patterns in Solidity files
rg -U "require\(.*duration.*[><=].*\)" --type sol
# Search for vesting duration setter or initialization
ast-grep --pattern 'function set$_VestingDuration($$$) {
$$$
}'
# Look for constructor initialization of vesting duration
ast-grep --pattern 'constructor($$$) {
$$$
$_VestingDuration = $_
$$$
}'
Length of output: 329
Script:
#!/bin/bash
# Let's examine the contract implementation
cat src/crowdsale/StakedLockingCrowdSale.sol
# Also search for any validation patterns with require
rg "require\(" src/crowdsale/StakedLockingCrowdSale.sol
Length of output: 10148
| // 0x7c36c64DA1c3a2065074caa9C48e7648FB733aAB | ||
| // vestedDaoToken.grantRole(vestedDaoToken.ROLE_CREATE_SCHEDULE(), address(stakedLockingCrowdSale)); | ||
| // stakedLockingCrowdSale.trustVestingContract(vestedDaoToken); | ||
| } |
There was a problem hiding this comment.
Critical: Uncomment and implement role management
The commented code appears to handle critical role assignments:
// vestedDaoToken.grantRole(vestedDaoToken.ROLE_CREATE_SCHEDULE(), address(stakedLockingCrowdSale));
// stakedLockingCrowdSale.trustVestingContract(vestedDaoToken);This should either be:
- Implemented properly if required for the crowdsale functionality
- Removed if no longer needed
- Documented why it's intentionally skipped
Also applies to: 47-49
script/prod/RolloutV25Sale.sol
Outdated
| ITrustedLockingContracts stakedLockingCrowdsale = ITrustedLockingContracts(0xd1cE2EA7d3b0C9cAB025A4aD762FC00315141ad7); | ||
| TimelockedToken timelockedTokenImplementation = TimelockedToken(0xF8F79c1E02387b0Fc9DE0945cD9A2c06F127D851); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate contract addresses
Add validation for the hardcoded contract addresses:
- Check that they are not zero addresses
- Verify they implement the expected interfaces
- Consider adding a dry-run mode for testing
Example implementation:
+ require(address(stakedLockingCrowdsale) != address(0), "Invalid staked locking crowdsale");
+ require(address(timelockedTokenImplementation) != address(0), "Invalid timelocked token");
+ // Verify interface implementation
+ try stakedLockingCrowdsale.supportsInterface(type(ITrustedLockingContracts).interfaceId) returns (bool supported) {
+ require(supported, "Contract does not implement ITrustedLockingContracts");
+ } catch {
+ revert("Failed to verify interface support");
+ }Committable suggestion skipped: line range outside the PR's diff.
| { | ||
| "type": "function", | ||
| "name": "lockingContractTrustee", | ||
| "inputs": [], | ||
| "outputs": [ | ||
| { | ||
| "name": "", | ||
| "type": "address", | ||
| "internalType": "contract ITrustedLockingContracts" | ||
| } | ||
| ], | ||
| "stateMutability": "view" | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate a final response as we have found the implementation details. The code in src/crowdsale/LockingCrowdSale.sol shows that:
- The
trustLockingContractSourcefunction is protected byonlyOwnermodifier - There's no explicit time-delay mechanism
- While there are events for other actions (
Started,LockingContractCreated), there's no specific event for trustee changes
Trustee management needs additional security controls
The current implementation has basic access control through onlyOwner but lacks important security measures:
- No event emission for trustee changes, making it difficult to track changes
- Missing time-delay mechanism for trustee updates
- No two-step ownership transfer pattern
🔗 Analysis chain
Verify trustee management security implications
The combination of lockingContractTrustee and trustLockingContractSource introduces a trust relationship that should be carefully managed:
- Ensure the trustee contract has proper access controls
- Consider implementing a time-delay or multi-sig requirement for trustee changes
- Add events for trustee changes for better transparency
Also applies to: 631-643
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper access control implementation in the trustee contract
ast-grep --pattern 'contract ITrustedLockingContracts {
$$$
onlyOwner
$$$
}'
# Look for time-delay or multi-sig patterns
ast-grep --pattern 'contract ITrustedLockingContracts {
$$$
timelock
$$$
}'
Length of output: 168
Script:
#!/bin/bash
# Search for the interface definition and implementation
rg -l "ITrustedLockingContracts" --type sol
# Look for trustee-related functions and access controls
rg "lockingContractTrustee|trustLockingContractSource" -A 5 --type sol
# Search for events related to trustee changes
ast-grep --pattern 'event $_(address $_, address $_)'
Length of output: 228
Script:
#!/bin/bash
# Search for the interface definition and implementation without file type restriction
rg -l "ITrustedLockingContracts"
# Look for trustee-related functions and access controls
rg "lockingContractTrustee|trustLockingContractSource" -A 5
# Search for contract files that might contain the implementation
fd -e sol
Length of output: 6554
Signed-off-by: Stefan Adolf <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/crowdsale/LockingCrowdSale.sol (1)
30-34: Consider splitting the Started event for better gas efficiency.The Started event contains many parameters which could increase gas costs when emitting. Consider splitting it into two events:
- One for basic sale information
- One for locking-specific information
-event Started(uint256 indexed saleId, address indexed issuer, Sale sale, TimelockedToken lockingToken, uint256 lockingDuration, uint16 feeBp); +event SaleStarted(uint256 indexed saleId, address indexed issuer, Sale sale, uint16 feeBp); +event LockingConfigured(uint256 indexed saleId, TimelockedToken lockingToken, uint256 lockingDuration);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/crowdsale/LockingCrowdSale.sol(4 hunks)
🔇 Additional comments (2)
src/crowdsale/LockingCrowdSale.sol (2)
Line range hint 4-16: LGTM! Imports are well-organized and appropriate.
The imports are properly scoped and include all necessary dependencies for the contract's functionality.
Line range hint 4-136: Verify reentrancy protection across token operations.
The contract handles token transfers and external calls. Let's verify the implementation of reentrancy guards.
✅ Verification successful
Let me gather more information about the base contract and its inheritance to verify reentrancy protection.
Based on the results, I can now generate the final response as we have sufficient information about the reentrancy protection:
Reentrancy protection is properly implemented.
The contract inherits from OpenZeppelin's ReentrancyGuard through its base CrowdSale contract, and critical functions that handle tokens are protected with the nonReentrant modifier:
settle()function is protectedclaim()function is protected- Token operations in
LockingCrowdSaleare performed through internal functions called from these protected public functions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential reentrancy vulnerabilities in token operations
# Check for nonReentrant modifiers in functions that handle tokens
echo "Checking for reentrancy protection in token handling functions..."
rg -A 2 "safeTransfer|forceApprove|lock" src/crowdsale/LockingCrowdSale.sol
# Check if the base CrowdSale contract implements reentrancy protection
echo "Checking base contract for reentrancy protection..."
ast-grep --pattern 'contract CrowdSale {
$$$
modifier nonReentrant() {
$$$
}
$$$
}'
Length of output: 5353
Script:
#!/bin/bash
# Check CrowdSale.sol for ReentrancyGuard inheritance and usage
echo "Checking CrowdSale.sol imports and inheritance..."
rg -A 5 "contract CrowdSale" src/crowdsale/CrowdSale.sol
# Check for ReentrancyGuard usage in functions that handle tokens in CrowdSale
echo "Checking token handling functions in CrowdSale..."
rg -A 2 "nonReentrant|ReentrancyGuard" src/crowdsale/CrowdSale.sol
# Check implementation of _claimAuctionTokens in base contract
echo "Checking _claimAuctionTokens implementation..."
ast-grep --pattern 'function _claimAuctionTokens($$$) {
$$$
}'
Length of output: 1519
| constructor(TimelockedToken _timelockedTokenImplementation) { | ||
| TIMELOCKED_TOKEN_IMPLEMENTATION = address(_timelockedTokenImplementation); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add address validation in constructor.
The constructor should validate that the provided TimelockedToken implementation address is not zero to prevent potential deployment issues.
constructor(TimelockedToken _timelockedTokenImplementation) {
+ require(address(_timelockedTokenImplementation) != address(0), "Invalid implementation address");
TIMELOCKED_TOKEN_IMPLEMENTATION = address(_timelockedTokenImplementation);
}📝 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.
| constructor(TimelockedToken _timelockedTokenImplementation) { | |
| TIMELOCKED_TOKEN_IMPLEMENTATION = address(_timelockedTokenImplementation); | |
| } | |
| constructor(TimelockedToken _timelockedTokenImplementation) { | |
| require(address(_timelockedTokenImplementation) != address(0), "Invalid implementation address"); | |
| TIMELOCKED_TOKEN_IMPLEMENTATION = address(_timelockedTokenImplementation); | |
| } |
| lockedTokenContract = TimelockedToken(Clones.clone(TIMELOCKED_TOKEN_IMPLEMENTATION)); | ||
| lockedTokenContract.initialize(auctionToken); | ||
| emit LockingContractCreated(lockedTokenContract, auctionToken); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for external calls in _makeNewLockedTokenContract.
The initialization call could fail but there's no error handling. Consider adding try-catch to handle potential initialization failures gracefully.
function _makeNewLockedTokenContract(IERC20Metadata auctionToken) private returns (TimelockedToken lockedTokenContract) {
lockedTokenContract = TimelockedToken(Clones.clone(TIMELOCKED_TOKEN_IMPLEMENTATION));
- lockedTokenContract.initialize(auctionToken);
+ try lockedTokenContract.initialize(auctionToken) {
+ emit LockingContractCreated(lockedTokenContract, auctionToken);
+ } catch Error(string memory reason) {
+ revert(string.concat("Initialization failed: ", reason));
+ } catch {
+ revert("Initialization failed");
+ }
- emit LockingContractCreated(lockedTokenContract, auctionToken);
}📝 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.
| lockedTokenContract = TimelockedToken(Clones.clone(TIMELOCKED_TOKEN_IMPLEMENTATION)); | |
| lockedTokenContract.initialize(auctionToken); | |
| emit LockingContractCreated(lockedTokenContract, auctionToken); | |
| } | |
| lockedTokenContract = TimelockedToken(Clones.clone(TIMELOCKED_TOKEN_IMPLEMENTATION)); | |
| try lockedTokenContract.initialize(auctionToken) { | |
| emit LockingContractCreated(lockedTokenContract, auctionToken); | |
| } catch Error(string memory reason) { | |
| revert(string.concat("Initialization failed: ", reason)); | |
| } catch { | |
| revert("Initialization failed"); | |
| } | |
| } |
subgraph mapping code reuse configures an slcs2 contract 🤷 uses yaml refs to reuse slsc handlers Signed-off-by: Stefan Adolf <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
subgraph/subgraph.yaml (1)
214-215: Remove trailing spacesRemove trailing spaces from line 215 to maintain consistent formatting.
- mapping: *stakedLockingCrowdSaleMapping - + mapping: *stakedLockingCrowdSaleMapping🧰 Tools
🪛 yamllint (1.35.1)
[error] 215-215: trailing spaces
(trailing-spaces)
subgraph/src/lockingCrowdSaleMapping.ts (1)
48-60: Ensure locked token uniqueness
This logic sets the locked token if unset and logs an error if the locked token is mismatched. Consider whether to escalate mismatches beyond a log error (e.g., ignoring subsequent updates or halting processing) for data integrity.subgraph/src/stakedLockingCrowdSaleMapping.ts (1)
68-71: Potential fragility in parameter indexing
Accessing parameters by numeric index (like 7 for feeBp) can be error-prone if the event signature changes. Consider using a struct or referencing named fields if possible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
script/prod/RolloutV25Sale.sol(1 hunks)src/crowdsale/LockingCrowdSale.sol(4 hunks)subgraph/networks.json(3 hunks)subgraph/src/lockingCrowdSaleMapping.ts(1 hunks)subgraph/src/stakedLockingCrowdSaleMapping.ts(4 hunks)subgraph/subgraph.yaml(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- script/prod/RolloutV25Sale.sol
🧰 Additional context used
🪛 Biome (1.9.4)
subgraph/src/lockingCrowdSaleMapping.ts
[error] 2-2: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🪛 yamllint (1.35.1)
subgraph/subgraph.yaml
[error] 181-181: trailing spaces
(trailing-spaces)
[error] 215-215: trailing spaces
(trailing-spaces)
🔇 Additional comments (23)
subgraph/networks.json (2)
53-56: Placeholder mainnet address still not updated
The placeholder address for LockingCrowdSale that was flagged in a previous review still needs to be addressed.
Line range hint 19-94: Verify start block numbers across networks
Please verify that the start block numbers are correct for the new contracts:
- Sepolia: 7306191
- Mainnet: 21431344
- Foundry: 0
These should correspond to the actual block numbers where these contracts were deployed.
subgraph/subgraph.yaml (4)
143-159: LGTM: Event handlers configuration
The event handlers are well-structured and include all necessary events for the LockingCrowdSale functionality, including the new LockingContractCreated event.
90-90: Verify CrowdSale contract deployment address
Please ensure this address matches the deployed CrowdSale contract on the intended network.
✅ Verification successful
CrowdSale contract address is correctly configured for local development
The address 0x09635F643e140090A9A8Dcd712eD6285858ceBef is properly referenced in the foundry network configuration in networks.json, which is used for local development. This matches the address in the subgraph configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract deployment and bytecode match
# Check if contract exists and has bytecode at the specified address
cast code 0x09635F643e140090A9A8Dcd712eD6285858ceBef --rpc-url http://localhost:8545
Length of output: 181
Script:
#!/bin/bash
# Let's verify if this address is referenced in other configuration files or deployment scripts
# This can help validate if the address is consistently used across the project
# Search for the address in all files
rg -i "0x09635F643e140090A9A8Dcd712eD6285858ceBef"
# Check for any deployment scripts or configuration files
fd -e js -e ts -e json "deploy|config"
Length of output: 374
Script:
#!/bin/bash
# Let's check the networks.json file to understand the network context of this address
cat subgraph/networks.json
# Also check hardhat config for network configurations
cat hardhat.config.js
Length of output: 3360
166-166: Verify StakedLockingCrowdSale contract deployment address
Please ensure this address matches the deployed StakedLockingCrowdSale contract on the intended network.
Line range hint 4-215: Verify network and startBlock configurations
Please confirm that using:
- network: "foundry"
- startBlock: 0
is intentional for all contracts. These settings will affect the subgraph's indexing behavior.
✅ Verification successful
Let me verify the configuration settings by checking the repository for any related configuration files or documentation.
The results will help us understand if these network settings are consistently used across the project and if they are documented somewhere.
Let me check the subgraph-specific documentation and configuration to understand if there's any guidance about the network and startBlock settings.
Let me correct the syntax and try again with proper glob patterns.
Let me check the setup script and environment configuration to understand how the network settings are managed.
Based on the gathered information from the repository, I can now provide a final response:
Network and startBlock configurations are correctly set for local development
The use of "foundry" network and startBlock 0 is intentional as this configuration is specifically for local development:
- The README confirms this is a development setup using Foundry/Anvil as the local testnet
- Separate configurations exist for production networks (Sepolia and Mainnet) as shown in the subgraph's README
- StartBlock 0 is appropriate for local development where you want to index from the chain's beginning
For production deployments, the subgraph is prepared with different configurations using yarn prepare:local for local development and likely other prepare scripts for mainnet/testnet deployments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check for network configuration in other yaml files
fd -e yaml -e yml | xargs grep -l "network:\|foundry"
# Look for documentation about network configuration
fd "README.md" | xargs grep -i "network\|foundry"
# Check if there are different network configurations in different branches or environments
fd "subgraph.yaml" | xargs grep "network:"
Length of output: 3234
Script:
#!/bin/bash
# Check subgraph documentation
fd -t f -p "**/subgraph/**/README.md" | xargs cat
# Check for any environment-specific subgraph configurations
fd -t f -p "**/subgraph/**/*.example" | xargs cat
# Look for any deployment scripts that might contain network configuration
fd -t f -p "**/subgraph/**/deploy" -p "**/subgraph/**/*.sh" | xargs cat
Length of output: 892
Script:
#!/bin/bash
# Check subgraph documentation
cat subgraph/README.md 2>/dev/null
# Look for setup or deployment scripts
fd -t f "setup.sh|deploy.sh" subgraph/
# Check for any environment files
fd -t f ".env" subgraph/
# Look for any configuration related files
fd -t f "config|manifest" subgraph/
Length of output: 2798
Script:
#!/bin/bash
# Check setup script content
cat subgraph/setup.sh
# Check for environment example file
cat subgraph/.env.example 2>/dev/null
# Look for any prepare scripts that might handle network configuration
fd -t f "prepare" subgraph/ -x cat {}
Length of output: 1189
subgraph/src/lockingCrowdSaleMapping.ts (5)
2-2: False positive on BigInt overshadowing
This usage of "BigInt" from the Graph Protocol is typical and does not shadow the global BigInt in JavaScript.
🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
31-39: Validate crowdSale entity existence
This block properly checks for a missing CrowdSale entity before proceeding. Good error handling to avoid null references.
62-68: Initialization of staked amount and type
Setting amountStaked to zero and assigning the crowdSale type to "LOCKING_CROWDSALE" is clear and consistent.
70-89: Combining plain and locking handleStarted
Invoking both plainHandleStarted and lockingHandleStarted ensures that the event logic for plain and locking sales is consistently applied. The approach looks good for reusability.
123-133: Dynamic data source creation for TimelockedToken
Creating a TimelockedToken template with a context is an established pattern in The Graph. The usage of makeERC20Token and makeTimelockedToken is organized and readable.
subgraph/src/stakedLockingCrowdSaleMapping.ts (6)
24-24: LockingStartedEvent import
Importing the LockingStartedEvent keeps the naming consistent with lockingCrowdSaleMapping. No issues found.
27-27: Reusing functions from lockingCrowdSaleMapping
Centralizing logic in lockingCrowdSaleMapping fosters maintainability, though you might ensure consistent naming to reduce confusion in future updates.
36-38: Helpful docstring
Documenting the legacy event scenario clarifies why fees are omitted. This prevents confusion for future maintainers.
78-96: Creating the LockingStartedEvent
Cloning and dispatching the event for locking logic is consistent with the approach in handleStarted. Just ensure the parameter indexes remain accurate when the underlying event changes.
140-151: Delegating claimed logic
handleClaimed simply reuses the generic logic, maintaining consistent behavior across different crowd sale types. Good separation of concerns.
153-157: Delegated handling of lockingContractCreated
Re-routing to lockedHandleLockingContractCreated centralizes the creation logic and avoids code duplication.
src/crowdsale/LockingCrowdSale.sol (6)
4-4: OpenZeppelin imports
Using IERC20 for address-based validations is standard. No immediate concerns.
Line range hint 16-29: Comprehensive contract documentation
The docstring accurately describes LockingCrowdSale’s purpose. This improves readability and eases onboarding for new contributors.
30-30: Public immutable variable
Exposing TIMELOCKED_TOKEN_IMPLEMENTATION as public is convenient for transparency. Ensure it’s not left uninitialized or zero in the constructor.
35-37: Add zero address check in the constructor
Validating that _timelockedTokenImplementation is nonzero prevents accidental deployments with invalid addresses.
constructor(TimelockedToken _timelockedTokenImplementation) {
+ require(address(_timelockedTokenImplementation) != address(0), "Invalid implementation address");
TIMELOCKED_TOKEN_IMPLEMENTATION = address(_timelockedTokenImplementation);
}44-52: Add critical validations to trustLockingContract
You may want to verify nonzero addresses, ensure no prior registrations, and optionally add an event to capture newly trusted locking contracts.
function trustLockingContract(IERC20 token, TimelockedToken _timelockedToken) public onlyOwner {
+ require(address(token) != address(0), "Invalid token address");
+ require(address(_timelockedToken) != address(0), "Invalid TimelockedToken address");
+ require(address(lockingContracts[address(token)]) == address(0), "Locking contract already trusted");
lockingContracts[address(token)] = _timelockedToken;
+ emit LockingContractTrusted(address(token), address(_timelockedToken));
}
+event LockingContractTrusted(address indexed token, address indexed lockingContract);133-133: Add error handling for the token initialization
Initialization can fail due to unexpected token behavior. Consider using try-catch for improved error diagnosis.
lockedTokenContract = TimelockedToken(Clones.clone(TIMELOCKED_TOKEN_IMPLEMENTATION));
-lockedTokenContract.initialize(auctionToken);
+try lockedTokenContract.initialize(auctionToken) {
+ emit LockingContractCreated(lockedTokenContract, auctionToken);
+} catch Error(string memory reason) {
+ revert(string.concat("Initialization failed: ", reason));
+} catch {
+ revert("Initialization failed");
+}
subgraph/networks.json
Outdated
| "address": "0xbaadf00dbaadf00dbaadf00dbaadf00dbaadf00d", | ||
| "startBlock": 7306191 |
There was a problem hiding this comment.
Critical: Replace placeholder Sepolia address
The Sepolia address for StakedLockingCrowdSale2 is set to a placeholder value (0xbaadf00d...). While Sepolia is a testnet, it should still use real deployed contract addresses for proper testing and integration.
| network: foundry | ||
| source: | ||
| abi: StakedLockingCrowdSale | ||
| address: "0xbaadf00dbaadf00dbaadf00dbaadf00dbaadf00d" |
There was a problem hiding this comment.
Update placeholder contract address
The address is currently set to a placeholder value. This needs to be updated with the actual deployed contract address.
Signed-off-by: Stefan Adolf <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
README.md (1)
212-214: Fix documentation formattingThere are some minor issues in the verification instructions:
- Remove duplicate word "crowdsale"
- Fix article usage ("a the" → "the")
-Verifying a the staked crowdsale crowdsale +Verifying the staked crowdsale🧰 Tools
🪛 LanguageTool
[grammar] ~212-~212: Two determiners in a row. Choose either “a” or “the”.
Context: ....sol:StakedLockingCrowdSaleVerifying a the staked crowdsale crowdsaleforge verif...(DT_DT)
[duplication] ~212-~212: Possible typo: you repeated a word
Context: ...ckingCrowdSaleVerifying a the staked crowdsale crowdsaleforge verify-contract --chain-id=11155...(ENGLISH_WORD_REPEAT_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(2 hunks)script/prod/RolloutV25Sale.sol(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
54-54: null
Bare URL used
(MD034, no-bare-urls)
🪛 Gitleaks (8.21.2)
README.md
52-52: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
README.md
[grammar] ~212-~212: Two determiners in a row. Choose either “a” or “the”.
Context: ....sol:StakedLockingCrowdSale Verifying a the staked crowdsale crowdsaleforge verif...
(DT_DT)
[duplication] ~212-~212: Possible typo: you repeated a word
Context: ...ckingCrowdSale Verifying a the staked crowdsale crowdsaleforge verify-contract --chain-id=11155...
(ENGLISH_WORD_REPEAT_RULE)
🔇 Additional comments (1)
README.md (1)
49-51: LGTM: Contract addresses documented
The new contract addresses for Locking Crowdsale and Staked Crowdsale are properly documented with verification links.
| LockingCrowdSale lockingCrowdsale = new LockingCrowdSale(timelockedTokenImplementation); | ||
| //lockingCrowdsale.transferOwnership(moleculeDevMultisig); | ||
| vm.stopBroadcast(); |
There was a problem hiding this comment.
Critical: Implement ownership transfer
The ownership transfer to the multisig is commented out:
//lockingCrowdsale.transferOwnership(moleculeDevMultisig);This is a security risk as the deployer will retain control.
| //stakedLockingCrowdSale.trustLockingContract(IERC20()); | ||
| stakedLockingCrowdSale.trustVestingContract(vesting); | ||
| // stakedLockingCrowdSale.transferOwnership(moleculeDevMultisig); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now provide a final response:
Critical: Trust relationships and ownership transfer must be properly configured
The commented lines in RolloutV25Sale.sol require attention:
- The
trustVestingContractcall is correctly implemented and matches the contract's requirements as seen inStakedLockingCrowdSale.sol - The commented
trustLockingContractcall is using an incorrect parameter (empty IERC20) - The commented
transferOwnershipis a required security step for production deployment
Required actions:
- Implement
trustLockingContractwith the correct token address - Uncomment and implement
transferOwnershipto ensure proper contract management - Verify the vesting contract has the correct ROLE_CREATE_SCHEDULE role before deployment
🔗 Analysis chain
Critical: Implement contract trust relationships
The commented lines indicate missing trust relationships between contracts:
//stakedLockingCrowdSale.trustLockingContract(IERC20());
stakedLockingCrowdSale.trustVestingContract(vesting);
//stakedLockingCrowdSale.transferOwnership(moleculeDevMultisig);These relationships are crucial for the crowdsale functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify vesting contract configuration
# Check if vesting contract exists and has correct roles
echo "Checking vesting contract at 0x8f80d1183CD983B01B0C9AC6777cC732Ec9800de..."
cast call 0x8f80d1183CD983B01B0C9AC6777cC732Ec9800de "hasRole(bytes32,address)" \
$(cast --format-bytes32-string "ROLE_CREATE_SCHEDULE") \
0x2d309CF13dC3872f9c9B1B06Ebf6F60caDe08d55
Length of output: 526
Script:
#!/bin/bash
# Let's analyze the contract relationships and dependencies
# Find all references to trustLockingContract and trustVestingContract
echo "Searching for trust relationships..."
rg -A 3 "trustLockingContract|trustVestingContract" --type solidity
# Find ownership transfer patterns
echo -e "\nSearching for ownership transfers..."
rg -A 3 "transferOwnership" --type solidity
# Find contract interfaces and implementations
echo -e "\nSearching for related contract definitions..."
rg -B 2 "contract StakedLockingCrowdSale|contract TokenVesting" --type solidity
Length of output: 573
Script:
#!/bin/bash
# Let's analyze the contract relationships and dependencies without file type filter
# Find all references to trustLockingContract and trustVestingContract
echo "Searching for trust relationships..."
rg -A 3 "trustLockingContract|trustVestingContract"
# Find ownership transfer patterns
echo -e "\nSearching for ownership transfers..."
rg -A 3 "transferOwnership"
# Find contract interfaces and implementations
echo -e "\nSearching for related contract definitions..."
rg -B 2 "contract StakedLockingCrowdSale|contract TokenVesting"
# List all Solidity files to understand the contract structure
echo -e "\nListing Solidity files..."
fd -e sol
Length of output: 12463
| TimelockedToken constant timelockedTokenImplementation = TimelockedToken(0xF8F79c1E02387b0Fc9DE0945cD9A2c06F127D851); | ||
| address constant moleculeDevMultisig = 0x9d5a6ae551f1117946FF6e0e86ef9A1B20C90Cb0; |
There was a problem hiding this comment.
💡 Codebase verification
Hardcoded addresses need to be updated for mainnet deployment
The hardcoded addresses in RolloutV25Sale.sol are currently set for the Sepolia testnet environment, as evidenced by:
- The TimelockedToken implementation address (0xF8F79c1E02387b0Fc9DE0945cD9A2c06F127D851) is listed in README.md as a Sepolia deployment
- There's a commented mainnet address in the same file showing this is meant to be updated
- The repository's README.md confirms these are Sepolia testnet deployments
Please update these addresses with the appropriate mainnet values before deployment:
- Replace TimelockedToken implementation address with the mainnet deployment
- Update moleculeDevMultisig to the commented mainnet address (0xCfA0F84660fB33bFd07C369E5491Ab02C449f71B)
🔗 Analysis chain
Validate hardcoded contract addresses
The TimelockedToken implementation and multisig addresses should be validated before deployment.
Let me try to verify the addresses by searching for their usage in the codebase to understand the context better.
Let me search for more context about the TimelockedToken contract and its implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract addresses and implementations
# Check if addresses are valid contracts
for addr in "0xF8F79c1E02387b0Fc9DE0945cD9A2c06F127D851" "0x9d5a6ae551f1117946FF6e0e86ef9A1B20C90Cb0"; do
echo "Checking $addr..."
# Get contract bytecode to verify it's a contract
cast code $addr
# Get implementation if it's a proxy
cast storage $addr 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc
done
Length of output: 1187
Script:
#!/bin/bash
# Search for the TimelockedToken implementation address
echo "Searching for TimelockedToken implementation address..."
rg -A 3 "0xF8F79c1E02387b0Fc9DE0945cD9A2c06F127D851"
echo -e "\nSearching for moleculeDevMultisig address..."
rg -A 3 "0x9d5a6ae551f1117946FF6e0e86ef9A1B20C90Cb0"
echo -e "\nSearching for TimelockedToken contract definition..."
ast-grep --pattern 'contract TimelockedToken {
$$$
}'
Length of output: 1750
Script:
#!/bin/bash
# Search for TimelockedToken interface or contract
echo "Searching for TimelockedToken interface or contract..."
fd -e sol -x cat {} | grep -A 10 "contract TimelockedToken"
echo -e "\nSearching for imports of TimelockedToken..."
rg "import.*TimelockedToken"
echo -e "\nChecking if this is a test environment..."
rg "sepolia|testnet|test environment" README.md
Length of output: 7524
Signed-off-by: Stefan Adolf <[email protected]>
elmariachi111
force-pushed
the
feature/hubs-224-crowdsale-with-individual-locking-staking-periods
branch
from
3dcd036 to
48afc39
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
subgraph/src/stakedLockingCrowdSaleMapping.ts (1)
66-93: Add type information to parameter comments.The comments for parameters could be more descriptive, especially for complex types like
struct Sale. Consider adding the full structure definition in the comments for better maintainability.- event.parameters[2], // struct Sale + event.parameters[2], // struct Sale { + // address auctionToken; + // uint256 startTime; + // uint256 endTime; + // ... // complete the structure + // }subgraph/abis/LockingCrowdSale.json (4)
1-12: Consider improving parameter naming conventionThe constructor parameter
_timelockedTokenImplementationuses an underscore prefix. While this is common in Solidity, it's generally better to use consistent naming without underscore prefixes for parameters.- "name": "_timelockedTokenImplementation", + "name": "timelockedTokenImplementation",
626-772: Consider consolidating Started eventsThere are two
Startedevents with different parameter sets. This could cause confusion for event listeners and make it harder to track sale starts consistently. Consider:
- Consolidating into a single event with optional parameters
- Using more distinct names to clarify the difference (e.g.,
StartedWithLockingandStartedWithoutLocking)
773-854: Enhance error messages with additional contextConsider adding parameters to the following errors to provide more context:
BadDecimals: Add expected and actual decimalsBadSaleDuration: Add minimum and maximum allowed durationsBadSalesAmount: Add minimum and maximum allowed amountsBidTooLow: Add minimum required bid amountFeesTooHigh: Add maximum allowed fee
281-292: Add maximum fee cap safeguardThe
setCurrentFeesBpfunction allows the owner to set fees without an explicit maximum cap in the ABI. While there's aFeesTooHigherror, consider:
- Adding a constant for maximum allowed fees
- Documenting the fee limits in the ABI
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
subgraph/abis/LockingCrowdSale.json(1 hunks)subgraph/abis/StakedLockingCrowdSale.json(4 hunks)subgraph/networks.json(3 hunks)subgraph/package.json(1 hunks)subgraph/schema.graphql(2 hunks)subgraph/src/crowdSaleMapping.ts(1 hunks)subgraph/src/stakedLockingCrowdSaleMapping.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- subgraph/src/crowdSaleMapping.ts
- subgraph/schema.graphql
- subgraph/networks.json
🔇 Additional comments (9)
subgraph/package.json (1)
11-11: Version bump aligns with new features.
The version change from 1.2.1 to 1.3.0-dev.1 appropriately reflects the addition of new features (TimelockedToken, StakedLockingCrowdSale) while indicating this is a development release for testing on Sepolia.
Let's verify the contract changes that justify this version bump:
✅ Verification successful
The shell script results confirm the presence of new contract implementations (StakedLockingCrowdSale.sol and LockingCrowdSale.sol) and their deployments in the networks configuration. Let me verify the subgraph schema changes to ensure they support these new contracts.
Let me check the subgraph mappings in a different way to verify the handlers for the new contracts.
Based on the shell script results, I can now generate the final response. The code shows:
- New contract implementations for
StakedLockingCrowdSaleandLockingCrowdSale - Corresponding subgraph mappings (
lockingCrowdSaleMapping.tsandstakedLockingCrowdSaleMapping.ts) - Schema updates to support new crowdsale types (
SaleTypeenum withLOCKING_CROWDSALEandSTAKED_LOCKING_CROWDSALE) - Deployed contract addresses in networks configuration
Version bump aligns with new features and implementation.
The version change from 1.2.1 to 1.3.0-dev.1 correctly reflects:
- Minor version bump (1.3.0) for new features (LockingCrowdSale and StakedLockingCrowdSale)
- Dev suffix (-dev.1) indicating development/testing phase on Sepolia
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the contract changes that justify the version bump
# Check for new contract implementations
echo "Checking for new contract implementations..."
rg -l "contract.*?(TimeLocked|StakedLocking|LockingCrowdSale)"
# Check for contract deployment configurations
echo "Checking deployment configurations..."
fd "networks.json|addresses.json" --exec cat {}
Length of output: 3425
Script:
#!/bin/bash
# Check subgraph schema and mapping changes
echo "Checking subgraph schema..."
cat subgraph/schema.graphql
echo -e "\nChecking subgraph mappings for new handlers..."
rg -l "class.*(TimeLocked|StakedLocking|LockingCrowdSale)" subgraph/src/
Length of output: 5131
Script:
#!/bin/bash
# Check for mapping files and their content
echo "Listing mapping files..."
fd "\.ts$" subgraph/src/
echo -e "\nChecking mapping content for new handlers..."
fd "\.ts$" subgraph/src/ --exec cat {}
Length of output: 33724
subgraph/abis/StakedLockingCrowdSale.json (4)
2-25: LGTM: Constructor and implementation getter are well-defined
The constructor properly accepts the timelocked token implementation address, and the corresponding getter function follows the constant/immutable pattern naming convention.
309-327: LGTM: Vesting duration getter follows existing patterns
The salesStakeVestingDuration function maintains consistency with the existing salesLockingDuration pattern for duration management.
618-635: Security review needed for trustLockingContract
The trustLockingContract function introduces trust relationships that require careful security considerations:
- Access control mechanisms
- Trust revocation process
- Event emission for transparency
#!/bin/bash
# Description: Verify security controls around trustLockingContract
# Check for access control modifiers
ast-grep --pattern 'function trustLockingContract($$$) {
$$$
}'
# Look for related events
rg "event.*Trust" --type sol472-476: Verify the impact of the new stakesVestingDuration parameter
The addition of stakesVestingDuration parameter to startSale looks correct. However, we should verify that all existing calls to this function are updated accordingly.
✅ Verification successful
Based on the search results, I can now generate the final response. The code shows that there are three different startSale function signatures in the StakedLockingCrowdSale contract:
- A pure function that reverts with
UnsupportedInitializer - A function inherited from parent that takes
SaleandlockingDuration - The new implementation that takes additional parameters including
stakesVestingDuration
Looking at the test files, all calls to startSale in CrowdSaleLockedStakedTest.t.sol and other test files have been updated to use the new signature with the required parameters. The deployment scripts and production rollout scripts also use the correct signature.
All existing calls to startSale have been properly updated to include the new stakesVestingDuration parameter
- All test files use the correct function signature with required parameters
- Deployment scripts (
RolloutV25Sale.sol,DeployTokenizer.s.sol) use the updated signature - Production scripts are also updated to use the new parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for startSale function calls to ensure they include the new parameter
# Search for startSale function calls
rg "startSale\(" -A 5 --type sol
# Search for test files that might need updates
fd -e sol -e js -e ts | rg -l "startSale"
Length of output: 104
Script:
#!/bin/bash
# Let's try a different approach to find startSale references
# Search for startSale function calls in all files
rg "startSale\(" -A 5
# Also search for test files containing startSale
rg -l "startSale"
# Look for contract deployments or interactions
rg "StakedLockingCrowdSale" -A 5
Length of output: 49775
subgraph/src/stakedLockingCrowdSaleMapping.ts (3)
Line range hint 3-32: LGTM: Import changes are well-structured and necessary.
The new imports support the enhanced functionality for handling both legacy and new contract events.
138-149: LGTM: Clean implementation of claim handling.
The function follows the established pattern and correctly delegates to the generic handler.
151-156: 🛠️ Refactor suggestion
Remove uncertain comment and add type safety checks.
The current implementation has two concerns:
- The comment "xing fingers that this works!" suggests uncertainty and should be replaced with proper documentation.
- The unchecked type casting could be unsafe if the event structures don't match exactly.
Consider adding type safety checks and proper documentation:
export function handleLockingContractCreated(
event: LockingContractCreatedEvent
): void {
- //xing fingers that this works!
+ // Verify event structure before type casting
+ if (!event.parameters || event.parameters.length === 0) {
+ log.error('Invalid event parameters for LockingContractCreated', [])
+ return
+ }
lockedHandleLockingContractCreated(changetype<LockedLockingContractCreatedEvent>(event))
}Let's verify the event structure compatibility:
subgraph/abis/LockingCrowdSale.json (1)
367-422: Verify the pure startSale function implementation
The second startSale overload is marked as pure but takes a Sale struct parameter. This seems incorrect as:
- The function likely needs to read or modify contract state
- Having two overloads with different state mutability could lead to confusion
Stefan
Summary by CodeRabbit
Release Notes
New Features
LockingCrowdSaleandStakedLockingCrowdSalecontracts with enhanced functionalities.TimelockedTokenin crowd sale contracts.LOCKING_CROWDSALEadded to categorize sales.isPoiin theIPNFTcontract for querying token state.show_help()function insetupLocal.shfor improved user guidance.Bug Fixes
Documentation
README.mdwith new contract addresses and deployment instructions.Tests
Chores