RNG Fallback #1782
RNG Fallback #1782
Conversation
WalkthroughThis update refactors the random number generation (RNG) system across contracts and deployment scripts. It replaces the old Changes
Sequence Diagram(s)sequenceDiagram
participant Governor
participant Consumer
participant RNG (IRNG)
participant SortitionModule
Governor->>RNG: changeConsumer(address)
Consumer->>RNG: requestRandomness()
RNG-->>Consumer: emits RequestSent event
RNG-->>RNG: (generates random number asynchronously)
Consumer->>RNG: receiveRandomness()
RNG-->>Consumer: returns random number (if ready)
Consumer->>SortitionModule: passPhase() or similar
SortitionModule->>RNG: requestRandomness()
RNG-->>SortitionModule: emits RequestSent event
SortitionModule->>RNG: receiveRandomness()
RNG-->>SortitionModule: returns random number
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-testnet failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
|
Code Climate has analyzed commit ec3cd79 and detected 11 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (9)
prettier-config/index.js (1)
25-25: Consider pinning the plugin versionTo ensure consistent formatting across different environments and prevent unexpected changes, consider importing the plugin with a specific version.
- plugins: [require("prettier-plugin-solidity")], + plugins: [require("[email protected]")],contracts/src/rng/RandomizerRNG.sol (1)
21-21: Specify visibility forlastRequestIdvariableThe variable
lastRequestIddoes not have an explicit visibility modifier. In Solidity, it's best practice to explicitly declare the visibility of state variables to avoid confusion.Apply this diff to specify the visibility:
- uint256 lastRequestId; // The last request ID. + uint256 public lastRequestId; // The last request ID.contracts/src/rng/IncrementalNG.sol (1)
11-11: Consider making the state variable privateThe
numbervariable could be made private since it's only used internally. This would prevent external contracts from directly accessing the current state.- uint256 public number; + uint256 private number;contracts/deploy/00-chainlink-rng.ts (1)
15-29: Consider environment-based key hash configurationHardcoding key hashes in the deployment script makes updates difficult. Consider moving these to environment variables or a configuration file.
// config/chainlink.json { "keyHashes": { "ARBITRUM_ONE": { "2": "0x9e9e...", // ... } } }contracts/deploy/00-home-chain-arbitration.ts (1)
Line range hint
113-119: Consider failing deployment on currency rate errorsSwallowing errors during currency rate configuration could lead to a partially initialized system. Consider either:
- Allowing the error to propagate to fail the deployment
- Adding a deployment flag to control whether currency rate errors should be fatal
- try { await changeCurrencyRate(core, await pnk.getAddress(), true, 12225583, 12); await changeCurrencyRate(core, await dai.getAddress(), true, 60327783, 11); await changeCurrencyRate(core, await weth.getAddress(), true, 1, 1); - } catch (e) { - console.error("failed to change currency rates:", e); - }contracts/src/rng/mock/ChainlinkVRFCoordinatorMock.sol (2)
35-56: Consider adding reentrancy protectionThe low-level call to the consumer could be exploited for reentrancy attacks. Although this is a mock contract, it's good practice to maintain security patterns.
function fulfillRandomWords(uint256 _requestId, address _consumer, uint256[] memory _words) public { + // Save and delete request before the external call to prevent reentrancy + VRFV2PlusClient.RandomWordsRequest memory req = requests[_requestId]; + delete requests[_requestId]; + if (requests[_requestId].subId == 0) { revert("nonexistent request"); } - VRFV2PlusClient.RandomWordsRequest memory req = requests[_requestId]; // ... rest of the function ... - delete (requests[_requestId]); emit RandomWordsFulfilled(_requestId, success); }
111-158: Consider making configuration values adjustableThe hardcoded configuration values might not cover all test scenarios. Consider making these values configurable through constructor parameters or setter functions to support diverse testing requirements.
+ uint16 private minimumRequestConfirmations = 3; + uint32 private maxGasLimit = 2000000; + + function setRequestConfig( + uint16 _minimumRequestConfirmations, + uint32 _maxGasLimit + ) external { + minimumRequestConfirmations = _minimumRequestConfirmations; + maxGasLimit = _maxGasLimit; + } function getRequestConfig() external view returns (uint16, uint32, bytes32[] memory) { - return (3, 2000000, new bytes32[](0)); + return (minimumRequestConfirmations, maxGasLimit, new bytes32[](0)); }contracts/deployments/arbitrumSepoliaDevnet/ChainlinkRNG.json (2)
216-228: Add events for critical parameter changesThe contract should emit events when critical parameters like keyHash and subscriptionId are modified to facilitate off-chain tracking and monitoring.
Add events for parameter changes:
+event KeyHashChanged(bytes32 oldKeyHash, bytes32 newKeyHash); +event SubscriptionIdChanged(uint256 oldSubId, uint256 newSubId); function changeKeyHash(bytes32 _keyHash) external onlyByGovernor { + emit KeyHashChanged(keyHash, _keyHash); keyHash = _keyHash; } function changeSubscriptionId(uint256 _subscriptionId) external onlyByGovernor { + emit SubscriptionIdChanged(subscriptionId, _subscriptionId); subscriptionId = _subscriptionId; }Also applies to: 242-254
558-568: Enhance constructor documentationThe constructor documentation should include information about the recommended ranges or constraints for the parameters.
Add parameter constraints to the documentation:
* @param _callbackGasLimit The limit for how much gas to use for the callback request to the contract's fulfillRandomWords() function. +* @dev Recommended range: 100,000-300,000 based on operation complexity * @param _requestConfirmations How many confirmations the Chainlink node should wait before responding. +* @dev Recommended range: 3-200 confirmations. Higher values increase security but also latency
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
contracts/deploy/00-chainlink-rng.ts(1 hunks)contracts/deploy/00-home-chain-arbitration-neo.ts(5 hunks)contracts/deploy/00-home-chain-arbitration-university.ts(3 hunks)contracts/deploy/00-home-chain-arbitration.ts(5 hunks)contracts/deploy/00-rng.ts(4 hunks)contracts/deploy/upgrade-sortition-module.ts(0 hunks)contracts/deployments/arbitrum/ChainlinkVRFCoordinator.json(1 hunks)contracts/deployments/arbitrumSepolia/ChainlinkVRFCoordinator.json(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/ChainlinkRNG.json(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/ChainlinkVRFCoordinator.json(1 hunks)contracts/package.json(1 hunks)contracts/src/arbitration/SortitionModule.sol(1 hunks)contracts/src/arbitration/SortitionModuleBase.sol(5 hunks)contracts/src/arbitration/SortitionModuleNeo.sol(1 hunks)contracts/src/rng/BlockhashRNG.sol(1 hunks)contracts/src/rng/ChainlinkRNG.sol(1 hunks)contracts/src/rng/IRNG.sol(1 hunks)contracts/src/rng/IncrementalNG.sol(1 hunks)contracts/src/rng/RNG.sol(0 hunks)contracts/src/rng/RNGWithFallback.sol(1 hunks)contracts/src/rng/RandomizerRNG.sol(6 hunks)contracts/src/rng/mock/ChainlinkVRFCoordinatorMock.sol(1 hunks)contracts/test/rng/index.ts(3 hunks)cspell.json(2 hunks)prettier-config/index.js(1 hunks)
💤 Files with no reviewable changes (2)
- contracts/src/rng/RNG.sol
- contracts/deploy/upgrade-sortition-module.ts
✅ Files skipped from review due to trivial changes (2)
- cspell.json
- contracts/deployments/arbitrum/ChainlinkVRFCoordinator.json
👮 Files not reviewed due to content moderation or server errors (3)
- contracts/src/arbitration/SortitionModuleBase.sol
- contracts/deployments/arbitrumSepolia/ChainlinkVRFCoordinator.json
- contracts/deployments/arbitrumSepoliaDevnet/ChainlinkVRFCoordinator.json
🔇 Additional comments (22)
prettier-config/index.js (1)
25-25: Verify prettier-plugin-solidity installation
The plugin needs to be available as a dev dependency for the configuration to work.
✅ Verification successful
Plugin is properly installed and available
The prettier-plugin-solidity is correctly installed as a dependency in both prettier-config/package.json and contracts/package.json with version ^1.4.1. The plugin will be available when Prettier runs since it's properly declared in the dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if prettier-plugin-solidity is properly installed
# Expected: Find the plugin in package.json and node_modules
# Check package.json for the dependency
echo "Checking package.json for prettier-plugin-solidity..."
jq '.devDependencies["prettier-plugin-solidity"]' package.json
# Check if the plugin exists in node_modules
echo "Checking if plugin directory exists..."
fd -t d "prettier-plugin-solidity$" node_modules
Length of output: 424
Script:
#!/bin/bash
# Let's check if package.json exists and examine its dependencies section
echo "Checking if package.json exists and its content..."
if [ -f "package.json" ]; then
cat package.json
else
echo "package.json not found"
fi
# Let's also check if there are any other package.json files in the repository
echo -e "\nSearching for all package.json files..."
fd "package.json$"
Length of output: 2874
Script:
#!/bin/bash
# Let's check the prettier-config package.json and its dependencies
echo "Checking prettier-config/package.json..."
cat prettier-config/package.json
# Also check if the plugin is mentioned in any other workspace package.json files
echo -e "\nChecking for prettier-plugin-solidity in other package.json files..."
rg "prettier-plugin-solidity" --type json
Length of output: 1003
contracts/src/rng/RNGWithFallback.sol (1)
86-109: Ensure secure governor address updates
When changing the governor via changeGovernor, it's crucial to validate the new address to prevent assigning an unintended or malicious address.
Run the following script to check for proper governor address assignment:
✅ Verification successful
Governor address update is properly secured
The changeGovernor function already implements the necessary security check by validating that the new governor address is not zero (require(_newGovernor != address(0), "Invalid governor")), and the function is protected by the onlyByGovernor modifier which ensures only the current governor can execute this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the governor address is not zero and is correctly updated.
# Search for instances where governor is set. Expect validations against zero address.
rg -A 2 'function changeGovernor' contracts/src/rng/RNGWithFallback.sol
Length of output: 247
contracts/src/rng/ChainlinkRNG.sol (1)
159-162: Confirm secure access control in fulfillRandomWords
While fulfillRandomWords relies on inherited access control, it's important to ensure that only the Chainlink VRF Coordinator can invoke it.
Run the following script to verify access control inheritance:
✅ Verification successful
Access control for fulfillRandomWords is properly implemented
The function is correctly protected through multiple security layers:
- It's declared as
internal override, meaning it can only be called from within the contract or inherited contracts - The contract inherits from
VRFConsumerBaseV2Plus, which implements the core access control inrawFulfillRandomWords - The implementation matches Chainlink's VRF v2.5 security model where only the verified VRF Coordinator can call this function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Confirm that fulfillRandomWords is correctly protected.
# Expect: fulfillRandomWords is declared internal and cannot be called externally.
ast-grep --pattern 'function fulfillRandomWords$_(uint256 $_, uint256[] calldata $_) internal override' contracts/src/rng/ChainlinkRNG.sol
Length of output: 140
Script:
#!/bin/bash
# Let's try a broader pattern to catch the function declaration
ast-grep --pattern 'function fulfillRandomWords' contracts/src/rng/ChainlinkRNG.sol
# Also get the full context of the contract to understand inheritance
cat contracts/src/rng/ChainlinkRNG.sol
Length of output: 7893
contracts/src/rng/IRNG.sol (1)
10-12:
Add view modifier to receiveRandomness
The receiveRandomness function should be marked as view since it does not modify state.
Apply this diff:
/// @dev Receive the random number.
/// @return randomNumber Random Number. If the number is not ready or has not been required 0 instead.
- function receiveRandomness() external returns (uint256 randomNumber);
+ function receiveRandomness() external view returns (uint256 randomNumber);Likely invalid or redundant comment.
contracts/src/rng/IncrementalNG.sol (1)
25-27: Verify unchecked arithmetic safety
The unchecked increment is safe here since:
- It's used for generating sequential numbers
- Even if it overflows, it would continue the sequence from 0
contracts/src/arbitration/SortitionModule.sol (2)
40-42: Storage layout changes require redeployment
The removal of _rngLookahead parameter and change from RNG to IRNG affects storage layout. As mentioned in PR objectives, this requires redeployment of the module.
✅ Verification successful
Based on the code analysis, I can now generate the final response:
Storage layout changes require redeployment - Confirmed
The removal of _rngLookahead parameter and change from RNG to IRNG affects storage layout as evidenced by:
- The
rngLookaheadstorage variable is marked as deprecated with a warning comment:// uint256 public rngLookahead; // Deprecated - WARNING: it breaks the storage layout of the contract, beta cannot be upgraded! - The base contract's storage layout has been modified by removing this variable.
- The initialization function signature has been changed to remove this parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify storage layout changes
# Check storage layout in current implementation
ast-grep --pattern 'contract SortitionModuleBase {
$$$
IRNG $_;
$$$
}'
# Check for any remaining references to RNG interface
rg "RNG[^I]" --type sol
Length of output: 132
Script:
#!/bin/bash
# Let's try a different approach to verify storage layout changes
# Check for RNG-related storage variables and inheritance
ast-grep --pattern 'contract SortitionModule$_? {
$$$
}'
# Look for storage variables in base contracts
ast-grep --pattern 'contract SortitionModuleBase {
$$$
}'
# Search for RNG-related code with proper file extension
rg "RNG[^I]" -t solidity
# Check inheritance chain
rg "contract SortitionModule.*is.*" -t solidity
Length of output: 265
Script:
#!/bin/bash
# Let's try without file type filtering since ripgrep doesn't recognize solidity extension
# Find all Solidity files first
fd "\.sol$" contracts/src/
# Then use these patterns on all files
rg "contract SortitionModule.*is.*"
rg "RNG[^I]"
# Look for storage-related changes in git diff
git diff origin/main -- contracts/src/arbitration/SortitionModule.sol
Length of output: 86560
Script:
#!/bin/bash
# Let's check the file content directly to verify storage layout changes
# Check the current file content
cat contracts/src/arbitration/SortitionModule.sol
# Check the base contract content
cat contracts/src/arbitration/SortitionModuleBase.sol
# Check the interface definition
cat contracts/src/arbitration/interfaces/ISortitionModule.sol
Length of output: 35127
Line range hint 48-52: Verify upgrade safety
The UUPS upgrade pattern is correctly implemented with governor-only access. However, ensure that all contracts in the inheritance chain are upgrade-safe.
contracts/deploy/00-rng.ts (2)
44-44: Verify network check logic change
The deployment skip condition has been modified to use isMainnet. Ensure this doesn't accidentally enable deployments on unintended networks.
✅ Verification successful
Let me analyze the network validation logic further by checking the implementation of these utility functions.
Network validation logic is safe and correctly implemented
The change to use isMainnet is actually more restrictive than the previous validation. The isMainnet function checks for the presence of a "production" tag, which means deployments will only run on networks explicitly tagged as production environments. This is safer than the previous approach that was using HomeChains enum check, as it adds an additional layer of protection against accidental deployments.
Key validation points:
isMainnetrelies on explicit production tags rather than chain IDs- The skip condition in the RNG deployment will prevent deployment on any network tagged as production
- Other deployment scripts use complementary validation with
HomeChainsenum for additional safety
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check network validation
# Look for network validation patterns
rg "isMainnet|HomeChains" --type ts
Length of output: 6938
Script:
#!/bin/bash
# Check the implementation of isSkipped and related network validation functions
cat contracts/deploy/utils/index.ts
Length of output: 1080
18-19: Verify SortitionModule deployment prerequisite
The script assumes SortitionModuleNeo is already deployed. Ensure deployment dependencies are properly ordered.
contracts/src/rng/BlockhashRNG.sol (2)
13-15:
Storage layout changes require careful migration
The contract's storage layout has changed significantly with the introduction of new state variables:
lookahead(immutable)requestBlockrandomNumbersmapping
This change makes the contract incompatible with upgrades and requires redeployment, as mentioned in the PR objectives.
23-24:
Potential race condition in requestRandomness
The function overwrites requestBlock without checking if there's an existing pending request. This could lead to a race condition if governance functions trigger a re-request.
contracts/src/arbitration/SortitionModuleNeo.sol (1)
50-54:
Storage layout changes require redeployment
The change from RNG to IRNG type and removal of _rngLookahead parameter alters the storage layout, making this contract incompatible with upgrades.
Let's verify the storage layout changes:
contracts/deploy/00-home-chain-arbitration-university.ts (2)
56-56: LGTM: Safe placeholder usage
Using ZeroAddress as a placeholder is acceptable here since the comment indicates that KlerosCore is configured later.
Line range hint 66-73: Improved core change handling with validation
Good improvement to check the current core before making changes, avoiding unnecessary transactions.
contracts/package.json (1)
107-109: Verify dependency versions for security
New dependencies added:
- @chainlink/contracts@^1.3.0
- @openzeppelin/contracts@^5.1.0
Let's check for known vulnerabilities:
✅ Verification successful
Based on the results from the verification scripts, I can now generate a final response:
Dependencies appear to be properly integrated and used
The verification shows:
- Chainlink contracts are used appropriately for VRF (Verifiable Random Function) implementation in
ChainlinkRNG.sol - OpenZeppelin contracts are widely used across the codebase in a standard way
- No immediate security concerns are visible in the implementation
The dependencies are:
- @chainlink/contracts@^1.3.0 - Used for VRF functionality
- @openzeppelin/contracts@^5.1.0 - Used for standard contract patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for known vulnerabilities in new dependencies
# Check npm for known vulnerabilities
npm audit --json @chainlink/contracts@^1.3.0 @openzeppelin/contracts@^5.1.0 | jq '.vulnerabilities'
# Check GitHub security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "chainlink__contracts") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 811
Script:
#!/bin/bash
# Let's try a different approach to check the dependencies
# First, let's check if these packages are actually used in the codebase
echo "=== Checking @chainlink/contracts usage ==="
rg -l "@chainlink/contracts" --type-add 'sol:*.sol'
echo -e "\n=== Checking @openzeppelin/contracts usage ==="
rg -l "@openzeppelin/contracts" --type-add 'sol:*.sol'
# Let's also check the package versions in the lock file if it exists
echo -e "\n=== Checking package-lock.json ==="
if [ -f "contracts/package-lock.json" ]; then
cat contracts/package-lock.json | jq '.packages["@chainlink/contracts"], .packages["@openzeppelin/contracts"]'
fi
# Let's check if there's a yarn.lock
echo -e "\n=== Checking yarn.lock ==="
if [ -f "contracts/yarn.lock" ]; then
cat contracts/yarn.lock | grep -A 5 "@chainlink/contracts"
echo "---"
cat contracts/yarn.lock | grep -A 5 "@openzeppelin/contracts"
fi
Length of output: 4908
Script:
#!/bin/bash
# Let's examine the actual usage of these dependencies in the code
echo "=== Checking Chainlink contracts usage patterns ==="
rg -C 2 "VRFCoordinator" --type-add 'sol:*.sol' contracts/
echo -e "\n=== Checking OpenZeppelin contracts usage patterns ==="
# Look for common OZ patterns
rg -l "import.*@openzeppelin/contracts/(access|token|utils|security)/" --type-add 'sol:*.sol' contracts/
# Check specific versions in use
echo -e "\n=== Checking current dependency versions ==="
if [ -f "contracts/package.json" ]; then
jq '.dependencies | select(has("@chainlink/contracts") or has("@openzeppelin/contracts"))' contracts/package.json
fi
# Let's also check the commit history for any recent security-related changes
echo -e "\n=== Checking recent security-related commits ==="
git log --oneline -n 10 contracts/package.json
Length of output: 66699
contracts/deploy/00-home-chain-arbitration.ts (2)
104-110: LGTM! Efficient configuration update
The conditional update of the RNG's consumer prevents unnecessary transactions and gas costs by checking the current state before making changes.
39-41: Verify initialization sequence for RandomizerRNG
The deployment uses ZeroAddress as a placeholder for SortitionModule, which is configured later. This is a safe approach, but we should verify that the contract cannot be used before proper initialization.
✅ Verification successful
Initialization sequence for RandomizerRNG is properly secured
The contract has adequate safeguards for initialization and usage:
- The initialize function properly sets the consumer address during initialization
- The
onlyByConsumermodifier ensures only the authorized consumer can request randomness - The consumer can only be changed by the governor through
changeConsumer - The
requestRandomnessfunction is protected byonlyByConsumermodifier - The callback function is protected by checking the sender is the randomizer contract
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if RandomizerRNG has safeguards against usage before initialization
ast-grep --pattern 'function $FUNC($$$) {
$$$
require($CONSUMER != address(0), $$$);
$$$
}'
Length of output: 99
Script:
#!/bin/bash
# Let's first find the RandomizerRNG contract implementation
fd -e sol -e vy "RandomizerRNG"
# Then examine its content for zero address checks and initialization patterns
rg -l "RandomizerRNG" --type sol
Length of output: 128
Script:
#!/bin/bash
# Search for require statements and modifiers in RandomizerRNG
rg "require\(" -A 2 -B 2 --type sol
Length of output: 65
Script:
#!/bin/bash
# Look for initialization or setup functions
ast-grep --pattern 'function set$_($$$) {
$$$
}'
Length of output: 52
Script:
#!/bin/bash
# Search for constructor implementation
ast-grep --pattern 'constructor($$$) {
$$$
}'
Length of output: 49
Script:
#!/bin/bash
# Let's examine the RandomizerRNG contract content directly
cat contracts/src/rng/RandomizerRNG.sol
Length of output: 5380
Script:
#!/bin/bash
# Search for require statements without file type specification
rg "require\(" -A 2 -B 2
Length of output: 66851
Script:
#!/bin/bash
# Look for functions that might use the consumer address
rg "consumer" -A 2 -B 2 contracts/src/rng/RandomizerRNG.sol
Length of output: 1011
contracts/deploy/00-home-chain-arbitration-neo.ts (1)
98-104: Same issues as in home-chain-arbitration.ts
This file contains the same patterns as 00-home-chain-arbitration.ts:
- The RNG consumer configuration pattern is correct
- The error handling for currency rates should be improved
Also applies to: 107-112
contracts/src/rng/mock/ChainlinkVRFCoordinatorMock.sol (2)
1-30: LGTM! Well-structured mock implementation
The contract properly implements the VRF coordinator interface with appropriate storage and events.
58-73: LGTM! Clean implementation
The request handling is straightforward and appropriate for testing purposes.
contracts/deployments/arbitrumSepoliaDevnet/ChainlinkRNG.json (2)
490-491: Review the callback gas limit configuration
The callback gas limit is set to 100,000 which might be insufficient for complex operations in the fulfillRandomWords function, especially if the consumer contract performs significant computation or storage operations.
Consider:
- Analyzing gas usage in test environments
- Implementing a safety margin (e.g., 200,000-300,000) to handle varying gas costs
- Documenting the gas requirement analysis
✅ Verification successful
Callback gas limit of 100,000 is sufficient for this implementation
The current callback gas limit of 100,000 is adequate for the ChainlinkRNG contract's fulfillRandomWords implementation because:
-
The function performs minimal operations:
- A single storage write to
randomNumbers[_requestId] - Emitting a single event with two parameters
- A single storage write to
-
The implementation has no complex computations, loops, or external calls that would require significant gas.
-
Similar storage operations typically consume around 20,000-25,000 gas, and event emissions around 2,000-3,000 gas, leaving a comfortable safety margin for the VRF callback.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check typical gas usage patterns for VRF callbacks
# Search for fulfillRandomWords implementations to analyze complexity
ast-grep --pattern 'function fulfillRandomWords($_, $_) {
$$$
}'
# Look for storage operations in fulfillRandomWords
rg -A 5 "fulfillRandomWords.*{"
Length of output: 208
Script:
#!/bin/bash
# Let's try a different approach to analyze the VRF callback implementation
# Search for fulfillRandomWords function implementation
ast-grep --pattern 'function fulfillRandomWords'
# Search for VRF-related files and contracts
fd -e sol -e json . | xargs rg -l "fulfillRandomWords"
# Check for gas-related configurations in source files
rg -i "callbackgas|gaslimit" -A 3
Length of output: 68149
Script:
#!/bin/bash
# Let's analyze the fulfillRandomWords implementation in ChainlinkRNG contract
# Search for fulfillRandomWords implementation
rg -p "function fulfillRandomWords.*\{" contracts/src/rng/ChainlinkRNG.sol -A 10
# Check for storage operations in the implementation
rg -p "randomNumbers\[.*\].*=" contracts/src/rng/ChainlinkRNG.sol
# Look for any gas-related comments or documentation
rg -p "gas|Gas" contracts/src/rng/ChainlinkRNG.sol
Length of output: 1771
484-491: Verify the governor configuration
The governor and sortitionModule are set to the same address in the constructor arguments, which might indicate a misconfiguration.
github-actions
bot
added
Package: Contracts
jaybuidl
force-pushed
the
feat/chainlink-vrf-as-rng2
branch
2 times, most recently
from
770d1c0 to
9e0f05b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
contracts/src/rng/BlockhashRNG.sol (1)
95-101: Consider improving randomness quality and transparency.The current implementation always uses
blockhash(block.number - 1)regardless of whenreceiveRandomness()is called within the time window. This could lead to predictable patterns. Additionally, there's no indication when randomness is unavailable (returns 0).Consider:
- Using a combination of block.timestamp and blockhash for better entropy
- Emitting events when randomness is unavailable
// Use last block hash for randomness - randomNumber = uint256(blockhash(block.number - 1)); + randomNumber = uint256(keccak256(abi.encodePacked(blockhash(block.number - 1), block.timestamp))); if (randomNumber != 0) { randomNumbers[expectedTimestamp] = randomNumber; + } else { + emit RandomnessUnavailable(expectedTimestamp); }contracts/src/rng/RNGWithFallback.sol (1)
169-173: Consider the impact of emergency reset on underlying RNG states.The
emergencyReset()function resets the contract's internal state but doesn't consider that underlying RNG contracts might still have pending requests. This could lead to unexpected behavior if those RNGs later provide responses.Consider adding a comment or documentation about the implications of emergency reset, or potentially add logic to notify the current RNG about the reset if possible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/deploy/00-home-chain-arbitration.ts(3 hunks)contracts/deploy/00-rng.ts(2 hunks)contracts/src/rng/BlockhashRNG.sol(1 hunks)contracts/src/rng/RNGWithFallback.sol(1 hunks)contracts/test/rng/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/deploy/00-home-chain-arbitration.ts
- contracts/deploy/00-rng.ts
- contracts/test/rng/index.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
contracts/src/rng/RNGWithFallback.sol (2)
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the contracts/src/rng/ChainlinkRNG.sol contract, the requestRandomness function is restricted to being called only by the SortitionModule, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
contracts/src/rng/BlockhashRNG.sol (2)
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the contracts/src/rng/ChainlinkRNG.sol contract, the requestRandomness function is restricted to being called only by the SortitionModule, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.
Learnt from: jaybuidl
PR: #1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the SortitionModule can call requestRandomness, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (4)
contracts/src/rng/BlockhashRNG.sol (3)
42-50: Constructor implementation looks correct.The previously reported constructor bug where
lookaheadwas being added to itself has been fixed. The implementation now correctly assigns the constructor parameters.
28-66: Access control implementation is secure and well-structured.The contract properly implements role-based access control with governor and consumer roles, including governance functions to update these roles.
107-119: View functions are well-implemented.The view functions properly handle edge cases and provide clear information about randomness readiness and availability timing.
contracts/src/rng/RNGWithFallback.sol (1)
1-185: Well-designed composite RNG with robust fallback mechanism.The contract implements a solid architectural pattern for RNG management with fallback capabilities. The design aligns well with the learned context that only the SortitionModule uses these functions without concurrent requests, making the single-request state management appropriate.
| constructor(address _governor, address _consumer, uint256 _fallbackTimeoutSeconds, IRNG _defaultRng) { | ||
| require(address(_defaultRng) != address(0), "Invalid default RNG"); | ||
|
|
||
| governor = _governor; | ||
| consumer = _consumer; | ||
| fallbackTimeoutSeconds = _fallbackTimeoutSeconds; | ||
| rngs.push(_defaultRng); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for governor and consumer addresses.
The constructor validates the default RNG but doesn't validate the governor and consumer addresses, which could lead to issues if zero addresses are passed.
constructor(address _governor, address _consumer, uint256 _fallbackTimeoutSeconds, IRNG _defaultRng) {
+ require(_governor != address(0), "Invalid governor");
+ require(_consumer != address(0), "Invalid consumer");
require(address(_defaultRng) != address(0), "Invalid default RNG");📝 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(address _governor, address _consumer, uint256 _fallbackTimeoutSeconds, IRNG _defaultRng) { | |
| require(address(_defaultRng) != address(0), "Invalid default RNG"); | |
| governor = _governor; | |
| consumer = _consumer; | |
| fallbackTimeoutSeconds = _fallbackTimeoutSeconds; | |
| rngs.push(_defaultRng); | |
| } | |
| constructor(address _governor, address _consumer, uint256 _fallbackTimeoutSeconds, IRNG _defaultRng) { | |
| require(_governor != address(0), "Invalid governor"); | |
| require(_consumer != address(0), "Invalid consumer"); | |
| require(address(_defaultRng) != address(0), "Invalid default RNG"); | |
| governor = _governor; | |
| consumer = _consumer; | |
| fallbackTimeoutSeconds = _fallbackTimeoutSeconds; | |
| rngs.push(_defaultRng); | |
| } |
🤖 Prompt for AI Agents
In contracts/src/rng/RNGWithFallback.sol around lines 42 to 49, the constructor
validates the default RNG address but does not validate the governor and
consumer addresses. Add require statements to check that both governor and
consumer addresses are not zero addresses to prevent invalid initialization.
This ensures the contract is deployed with valid critical addresses.
| // If the timeout is exceeded, try next RNG | ||
| if (block.timestamp > requestTimestamp + fallbackTimeoutSeconds) { | ||
| uint256 nextIndex = currentRngIndex + 1; | ||
|
|
||
| // If we have another RNG to try, switch to it and request again | ||
| if (nextIndex < rngs.length) { | ||
| emit RNGFallback(currentRngIndex, nextIndex); | ||
| currentRngIndex = nextIndex; | ||
| _requestRandomness(nextIndex); | ||
| } else { | ||
| // No more RNGs to try | ||
| emit RNGFailure(); | ||
| } | ||
| } | ||
| return randomNumber; |
There was a problem hiding this comment.
Fix timeout logic for fallback RNG requests.
When switching to a fallback RNG, the code doesn't reset requestTimestamp, causing subsequent timeout checks to be based on the original request time rather than the new fallback request time. This could lead to immediate timeouts for subsequent fallbacks.
// If the timeout is exceeded, try next RNG
if (block.timestamp > requestTimestamp + fallbackTimeoutSeconds) {
uint256 nextIndex = currentRngIndex + 1;
// If we have another RNG to try, switch to it and request again
if (nextIndex < rngs.length) {
emit RNGFallback(currentRngIndex, nextIndex);
- currentRngIndex = nextIndex;
- _requestRandomness(nextIndex);
+ _requestRandomness(nextIndex); // This will update requestTimestamp
} else {
// No more RNGs to try
emit RNGFailure();
}
}📝 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.
| // If the timeout is exceeded, try next RNG | |
| if (block.timestamp > requestTimestamp + fallbackTimeoutSeconds) { | |
| uint256 nextIndex = currentRngIndex + 1; | |
| // If we have another RNG to try, switch to it and request again | |
| if (nextIndex < rngs.length) { | |
| emit RNGFallback(currentRngIndex, nextIndex); | |
| currentRngIndex = nextIndex; | |
| _requestRandomness(nextIndex); | |
| } else { | |
| // No more RNGs to try | |
| emit RNGFailure(); | |
| } | |
| } | |
| return randomNumber; | |
| // If the timeout is exceeded, try next RNG | |
| if (block.timestamp > requestTimestamp + fallbackTimeoutSeconds) { | |
| uint256 nextIndex = currentRngIndex + 1; | |
| // If we have another RNG to try, switch to it and request again | |
| if (nextIndex < rngs.length) { | |
| emit RNGFallback(currentRngIndex, nextIndex); | |
| _requestRandomness(nextIndex); // This will update requestTimestamp | |
| } else { | |
| // No more RNGs to try | |
| emit RNGFailure(); | |
| } | |
| } | |
| return randomNumber; |
🤖 Prompt for AI Agents
In contracts/src/rng/RNGWithFallback.sol around lines 94 to 108, the timeout
logic for fallback RNG requests does not reset the requestTimestamp after
switching to a fallback RNG, causing subsequent timeout checks to use the
original request time. To fix this, update the code to reset requestTimestamp to
the current block.timestamp immediately after switching to the fallback RNG and
making the new request, ensuring timeout checks are based on the new request
time.
| function removeLastRngFallback() external onlyByGovernor { | ||
| require(rngs.length > 1, "No fallback RNG"); | ||
|
|
||
| // If the removed RNG is the current one, reset the fallback index | ||
| if (currentRngIndex > rngs.length - 2) { | ||
| currentRngIndex = DEFAULT_RNG; | ||
| } | ||
|
|
||
| IRNG removedRng = rngs[rngs.length - 1]; | ||
| rngs.pop(); | ||
| emit RNGFallbackRemoved(address(removedRng)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle active requests when removing the current RNG.
If the currently active RNG is removed while a request is in progress, the contract should reset the request state to avoid inconsistencies.
// If the removed RNG is the current one, reset the fallback index
if (currentRngIndex > rngs.length - 2) {
+ // If we're removing the current RNG and there's an active request, reset state
+ if (isRequesting) {
+ isRequesting = false;
+ requestTimestamp = 0;
+ }
currentRngIndex = DEFAULT_RNG;
}📝 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.
| function removeLastRngFallback() external onlyByGovernor { | |
| require(rngs.length > 1, "No fallback RNG"); | |
| // If the removed RNG is the current one, reset the fallback index | |
| if (currentRngIndex > rngs.length - 2) { | |
| currentRngIndex = DEFAULT_RNG; | |
| } | |
| IRNG removedRng = rngs[rngs.length - 1]; | |
| rngs.pop(); | |
| emit RNGFallbackRemoved(address(removedRng)); | |
| } | |
| function removeLastRngFallback() external onlyByGovernor { | |
| require(rngs.length > 1, "No fallback RNG"); | |
| // If the removed RNG is the current one, reset the fallback index | |
| if (currentRngIndex > rngs.length - 2) { | |
| // If we're removing the current RNG and there's an active request, reset state | |
| if (isRequesting) { | |
| isRequesting = false; | |
| requestTimestamp = 0; | |
| } | |
| currentRngIndex = DEFAULT_RNG; | |
| } | |
| IRNG removedRng = rngs[rngs.length - 1]; | |
| rngs.pop(); | |
| emit RNGFallbackRemoved(address(removedRng)); | |
| } |
🤖 Prompt for AI Agents
In contracts/src/rng/RNGWithFallback.sol around lines 147 to 158, the function
removeLastRngFallback does not handle the case where the current RNG is removed
while a request is active. To fix this, add logic to check if there is an active
request when removing the current RNG and reset the request state accordingly to
prevent inconsistencies. This may involve clearing or resetting any variables
that track the request status before or after updating the currentRngIndex and
removing the RNG.
|
Superseded by #2054 |
This PR is a redo of #966 as it has deviated too much from dev. It is based atop the
feat/chainlink-vrf-as-rng2branch (PR #1778).This PR introduces a composite RNG called
RNGWithFallbackresponsible for the fallback logic.This PR breaks the storage layout of the SortitionModule so it cannot be upgraded and will require a full redeploy.
The RNGs are permissioned to prevent 3rd parties from using up the funds on the RNG oracle accounts with Chainlink or Randomizer.
The RNGs do not support concurrent requests, it is not necessary since only 1 consumer is allowed (the sortition module), with the notable exception of some governance function triggering a re-request, which invalidates the previous one.
This PR introduces several ABI changes:
PR-Codex overview
This PR focuses on refactoring the random number generation system by introducing the
IRNGinterface, modifying contracts to utilize this interface, and updating deployment scripts accordingly. It enhances flexibility and standardizes random number generation across various implementations.Detailed summary
RNG.solcspell.json.IRNGinterface incontracts/src/rng/IRNG.sol.SortitionModuleandSortitionModuleNeoto useIRNGinstead ofRNG.RNG_LOOKAHEADtoRNG_LOOKAHEAD_TIMEin deployment scripts.IncrementalNG,RandomizerRNG,ChainlinkRNG, andBlockHashRNGto implementIRNG.RNGWithFallbackcontract to provide a fallback mechanism for RNG requests.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests