Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RNG Fallback #1782

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from
Draft

RNG Fallback #1782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ff6097
feat: composite rng with fallback logic, removal of the block param f…
jaybuidl Dec 9, 2024
586006d
refactor: renamed RNG into IRNG
jaybuidl Dec 9, 2024
ec3cd79
chore: sortition module lookahead state var removal
jaybuidl Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 3 additions & 13 deletions contracts/deploy/00-home-chain-arbitration-neo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment)
const { ethers, deployments, getNamedAccounts, getChainId } = hre;
const { deploy, execute } = deployments;
const { ZeroAddress } = hre.ethers;
const RNG_LOOKAHEAD = 20;

// fallback to hardhat node signers on local network
const deployer = (await getNamedAccounts()).deployer ?? (await hre.ethers.getSigners())[0].address;
Expand Down Expand Up @@ -62,16 +61,7 @@ const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment)
const maxTotalStaked = PNK(2_000_000);
const sortitionModule = await deployUpgradable(deployments, "SortitionModuleNeo", {
from: deployer,
args: [
deployer,
klerosCoreAddress,
minStakingTime,
maxFreezingTime,
rng.address,
RNG_LOOKAHEAD,
maxStakePerJuror,
maxTotalStaked,
],
args: [deployer, klerosCoreAddress, minStakingTime, maxFreezingTime, rng.address, maxStakePerJuror, maxTotalStaked],
log: true,
}); // nonce (implementation), nonce+1 (proxy)

Expand Down Expand Up @@ -107,10 +97,10 @@ const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment)

// rng.changeSortitionModule() only if necessary
const rngContract = (await ethers.getContract("RandomizerRNG")) as RandomizerRNG;
const currentSortitionModule = await rngContract.sortitionModule();
const currentSortitionModule = await rngContract.consumer();
if (currentSortitionModule !== sortitionModule.address) {
console.log(`rng.changeSortitionModule(${sortitionModule.address})`);
await rngContract.changeSortitionModule(sortitionModule.address);
await rngContract.changeConsumer(sortitionModule.address);
}

const core = (await hre.ethers.getContract("KlerosCoreNeo")) as KlerosCoreNeo;
Expand Down
7 changes: 3 additions & 4 deletions contracts/deploy/00-home-chain-arbitration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { DisputeKitClassic, KlerosCore, RandomizerRNG } from "../typechain-types
const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {
const { ethers, deployments, getNamedAccounts, getChainId } = hre;
const { ZeroAddress } = hre.ethers;
const RNG_LOOKAHEAD = 20;

// fallback to hardhat node signers on local network
const deployer = (await getNamedAccounts()).deployer ?? (await hre.ethers.getSigners())[0].address;
Expand Down Expand Up @@ -69,7 +68,7 @@ const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment)
const maxFreezingTime = devnet ? 600 : 1800;
const sortitionModule = await deployUpgradable(deployments, "SortitionModule", {
from: deployer,
args: [deployer, klerosCoreAddress, minStakingTime, maxFreezingTime, rng.target, RNG_LOOKAHEAD],
args: [deployer, klerosCoreAddress, minStakingTime, maxFreezingTime, rng.target],
log: true,
}); // nonce (implementation), nonce+1 (proxy)

Expand Down Expand Up @@ -104,10 +103,10 @@ const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment)

// rng.changeSortitionModule() only if necessary
const rngContract = (await ethers.getContract("RandomizerRNG")) as RandomizerRNG;
const currentSortitionModule = await rngContract.sortitionModule();
const currentSortitionModule = await rngContract.consumer();
if (currentSortitionModule !== sortitionModule.address) {
console.log(`rng.changeSortitionModule(${sortitionModule.address})`);
await rngContract.changeSortitionModule(sortitionModule.address);
await rngContract.changeConsumer(sortitionModule.address);
}

const core = (await hre.ethers.getContract("KlerosCore")) as KlerosCore;
Expand Down
2 changes: 0 additions & 2 deletions contracts/deploy/upgrade-sortition-module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { HomeChains, isSkipped } from "./utils";

const deployUpgradeSortitionModule: DeployFunction = async (hre: HardhatRuntimeEnvironment) => {
const { deployments, getNamedAccounts, getChainId } = hre;
const RNG_LOOKAHEAD = 20;

// fallback to hardhat node signers on local network
const deployer = (await getNamedAccounts()).deployer ?? (await hre.ethers.getSigners())[0].address;
Expand All @@ -26,7 +25,6 @@ const deployUpgradeSortitionModule: DeployFunction = async (hre: HardhatRuntimeE
1800, // minStakingTime
1800, // maxFreezingTime
rng.address,
RNG_LOOKAHEAD,
],
});
} catch (err) {
Expand Down
6 changes: 2 additions & 4 deletions contracts/src/arbitration/SortitionModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,14 @@ contract SortitionModule is SortitionModuleBase, UUPSProxiable, Initializable {
/// @param _minStakingTime Minimal time to stake
/// @param _maxDrawingTime Time after which the drawing phase can be switched
/// @param _rng The random number generator.
/// @param _rngLookahead Lookahead value for rng.
function initialize(
address _governor,
KlerosCore _core,
uint256 _minStakingTime,
uint256 _maxDrawingTime,
RNG _rng,
uint256 _rngLookahead
IRNG _rng
) external reinitializer(1) {
super._initialize(_governor, _core, _minStakingTime, _maxDrawingTime, _rng, _rngLookahead);
super._initialize(_governor, _core, _minStakingTime, _maxDrawingTime, _rng);
}

// ************************************* //
Expand Down
27 changes: 10 additions & 17 deletions contracts/src/arbitration/SortitionModuleBase.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pragma solidity 0.8.24;
import "./KlerosCore.sol";
import "./interfaces/ISortitionModule.sol";
import "./interfaces/IDisputeKit.sol";
import "../rng/RNG.sol";
import "../rng/IRNG.sol";
import "../libraries/Constants.sol";

/// @title SortitionModuleBase
Expand Down Expand Up @@ -63,11 +63,10 @@ abstract contract SortitionModuleBase is ISortitionModule {
uint256 public minStakingTime; // The time after which the phase can be switched to Drawing if there are open disputes.
uint256 public maxDrawingTime; // The time after which the phase can be switched back to Staking.
uint256 public lastPhaseChange; // The last time the phase was changed.
uint256 public randomNumberRequestBlock; // Number of the block when RNG request was made.
uint256 public disputesWithoutJurors; // The number of disputes that have not finished drawing jurors.
RNG public rng; // The random number generator.
IRNG public rng; // The random number generator.
uint256 public randomNumber; // Random number returned by RNG.
uint256 public rngLookahead; // Minimal block distance between requesting and obtaining a random number.
// uint256 public rngLookahead; // Deprecated - WARNING: it breaks the storage layout of the contract, beta cannot be upgraded!
uint256 public delayedStakeWriteIndex; // The index of the last `delayedStake` item that was written to the array. 0 index is skipped.
uint256 public delayedStakeReadIndex; // The index of the next `delayedStake` item that should be processed. Starts at 1 because 0 index is skipped.
mapping(bytes32 treeHash => SortitionSumTree) sortitionSumTrees; // The mapping trees by keys.
Expand All @@ -94,16 +93,14 @@ abstract contract SortitionModuleBase is ISortitionModule {
KlerosCore _core,
uint256 _minStakingTime,
uint256 _maxDrawingTime,
RNG _rng,
uint256 _rngLookahead
IRNG _rng
) internal {
governor = _governor;
core = _core;
minStakingTime = _minStakingTime;
maxDrawingTime = _maxDrawingTime;
lastPhaseChange = block.timestamp;
rng = _rng;
rngLookahead = _rngLookahead;
delayedStakeReadIndex = 1;
}

Expand Down Expand Up @@ -137,15 +134,12 @@ abstract contract SortitionModuleBase is ISortitionModule {
maxDrawingTime = _maxDrawingTime;
}

/// @dev Changes the `_rng` and `_rngLookahead` storage variables.
/// @param _rng The new value for the `RNGenerator` storage variable.
/// @param _rngLookahead The new value for the `rngLookahead` storage variable.
function changeRandomNumberGenerator(RNG _rng, uint256 _rngLookahead) external onlyByGovernor {
/// @dev Changes the `rng` storage variable.
/// @param _rng The new random number generator.
function changeRandomNumberGenerator(IRNG _rng) external onlyByGovernor {
rng = _rng;
rngLookahead = _rngLookahead;
if (phase == Phase.generating) {
rng.requestRandomness(block.number + rngLookahead);
randomNumberRequestBlock = block.number;
rng.requestRandomness();
}
}

Expand All @@ -160,11 +154,10 @@ abstract contract SortitionModuleBase is ISortitionModule {
"The minimum staking time has not passed yet."
);
require(disputesWithoutJurors > 0, "There are no disputes that need jurors.");
rng.requestRandomness(block.number + rngLookahead);
randomNumberRequestBlock = block.number;
rng.requestRandomness();
phase = Phase.generating;
} else if (phase == Phase.generating) {
randomNumber = rng.receiveRandomness(randomNumberRequestBlock + rngLookahead);
randomNumber = rng.receiveRandomness();
require(randomNumber != 0, "Random number is not ready yet");
phase = Phase.drawing;
} else if (phase == Phase.drawing) {
Expand Down
6 changes: 2 additions & 4 deletions contracts/src/arbitration/SortitionModuleNeo.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,20 +40,18 @@ contract SortitionModuleNeo is SortitionModuleBase, UUPSProxiable, Initializable
/// @param _minStakingTime Minimal time to stake
/// @param _maxDrawingTime Time after which the drawing phase can be switched
/// @param _rng The random number generator.
/// @param _rngLookahead Lookahead value for rng.
/// @param _maxStakePerJuror The maximum amount of PNK a juror can stake in a court.
/// @param _maxTotalStaked The maximum amount of PNK that can be staked in all courts.
function initialize(
address _governor,
KlerosCore _core,
uint256 _minStakingTime,
uint256 _maxDrawingTime,
RNG _rng,
uint256 _rngLookahead,
IRNG _rng,
uint256 _maxStakePerJuror,
uint256 _maxTotalStaked
) external reinitializer(2) {
super._initialize(_governor, _core, _minStakingTime, _maxDrawingTime, _rng, _rngLookahead);
super._initialize(_governor, _core, _minStakingTime, _maxDrawingTime, _rng);
maxStakePerJuror = _maxStakePerJuror;
maxTotalStaked = _maxTotalStaked;
}
Expand Down
30 changes: 17 additions & 13 deletions contracts/src/rng/BlockhashRNG.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,43 +2,47 @@

pragma solidity 0.8.24;

import "./RNG.sol";
import "./IRNG.sol";

/// @title Random Number Generator using blockhash with fallback.
/// @author Clément Lesaege - <[email protected]>
/// @dev
/// Random Number Generator returning the blockhash with a fallback behaviour.
/// In case no one called it within the 256 blocks, it returns the previous blockhash.
/// This contract must be used when returning 0 is a worse failure mode than returning another blockhash.
/// Allows saving the random number for use in the future. It allows the contract to still access the blockhash even after 256 blocks.
contract BlockHashRNG is RNG {
contract BlockHashRNG is IRNG {
uint256 public immutable lookahead; // Minimal block distance between requesting and obtaining a random number.
uint256 public requestBlock; // Block number of the current request
mapping(uint256 block => uint256 number) public randomNumbers; // randomNumbers[block] is the random number for this block, 0 otherwise.

constructor(uint256 _lookahead) {
lookahead = _lookahead + lookahead;
}
Comment on lines +18 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential initialization bug in constructor

There's a bug in the constructor where lookahead is being added to itself, which would double the intended value.

-        lookahead = _lookahead + lookahead;
+        lookahead = _lookahead;
📝 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.

Suggested change
constructor(uint256 _lookahead) {
lookahead = _lookahead + lookahead;
}
constructor(uint256 _lookahead) {
lookahead = _lookahead;
}


/// @dev Request a random number.
/// @param _block Block the random number is linked to.
function requestRandomness(uint256 _block) external override {
// nop
function requestRandomness() external override {
requestBlock = block.number;
}

/// @dev Return the random number. If it has not been saved and is still computable compute it.
/// @param _block Block the random number is linked to.
/// @return randomNumber The random number or 0 if it is not ready or has not been requested.
function receiveRandomness(uint256 _block) external override returns (uint256 randomNumber) {
randomNumber = randomNumbers[_block];
function receiveRandomness() external override returns (uint256 randomNumber) {
uint256 expectedBlock = requestBlock;
randomNumber = randomNumbers[expectedBlock];
if (randomNumber != 0) {
return randomNumber;
}

if (_block < block.number) {
if (expectedBlock < block.number) {
// The random number is not already set and can be.
if (blockhash(_block) != 0x0) {
if (blockhash(expectedBlock) != 0x0) {
// Normal case.
randomNumber = uint256(blockhash(_block));
randomNumber = uint256(blockhash(expectedBlock));
} else {
// The contract was not called in time. Fallback to returning previous blockhash.
randomNumber = uint256(blockhash(block.number - 1));
}
}
randomNumbers[_block] = randomNumber;
randomNumbers[expectedBlock] = randomNumber;
Comment on lines +29 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Review fallback mechanism security implications

The fallback mechanism using the previous blockhash when the original block is no longer available has several security implications:

  1. Predictability: The previous blockhash is more predictable than the intended block
  2. Potential manipulation: Malicious actors could delay calls to force fallback
  3. No event emission on fallback: Makes it harder to track when fallback was used

Consider:

  1. Emitting an event when falling back
  2. Adding a maximum delay threshold
 if (blockhash(expectedBlock) != 0x0) {
     // Normal case.
     randomNumber = uint256(blockhash(expectedBlock));
 } else {
     // The contract was not called in time. Fallback to returning previous blockhash.
+    emit FallbackUsed(expectedBlock, block.number - 1);
     randomNumber = uint256(blockhash(block.number - 1));
 }

Committable suggestion skipped: line range outside the PR's diff.

}
}
40 changes: 20 additions & 20 deletions contracts/src/rng/ChainlinkRNG.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,17 @@ pragma solidity 0.8.24;
import {VRFConsumerBaseV2Plus, IVRFCoordinatorV2Plus} from "@chainlink/contracts/src/v0.8/vrf/dev/VRFConsumerBaseV2Plus.sol";
import {VRFV2PlusClient} from "@chainlink/contracts/src/v0.8/vrf/dev/libraries/VRFV2PlusClient.sol";

import "./RNG.sol";
import "./IRNG.sol";

/// @title Random Number Generator that uses Chainlink VRF v2.5
/// https://blog.chain.link/introducing-vrf-v2-5/
contract ChainlinkRNG is RNG, VRFConsumerBaseV2Plus {
contract ChainlinkRNG is IRNG, VRFConsumerBaseV2Plus {
// ************************************* //
// * Storage * //
// ************************************* //

address public governor; // The address that can withdraw funds.
address public sortitionModule; // The address of the SortitionModule.
address public consumer; // The address that can request random numbers.
bytes32 public keyHash; // The gas lane key hash value - Defines the maximum gas price you are willing to pay for a request in wei (ID of the off-chain VRF job).
uint256 public subscriptionId; // The unique identifier of the subscription used for funding requests.
uint16 public requestConfirmations; // How many confirmations the Chainlink node should wait before responding.
Expand All @@ -29,13 +29,13 @@ contract ChainlinkRNG is RNG, VRFConsumerBaseV2Plus {
// ************************************* //

/// @dev Emitted when a request is sent to the VRF Coordinator
/// @param requestId The ID of the request
event RequestSent(uint256 indexed requestId);
/// @param _requestId The ID of the request
event RequestSent(uint256 indexed _requestId);

/// Emitted when a request has been fulfilled.
/// @param requestId The ID of the request
/// @param randomWord The random value answering the request.
event RequestFulfilled(uint256 indexed requestId, uint256 randomWord);
/// @param _requestId The ID of the request
/// @param _randomWord The random value answering the request.
event RequestFulfilled(uint256 indexed _requestId, uint256 _randomWord);

// ************************************* //
// * Function Modifiers * //
Expand All @@ -46,8 +46,8 @@ contract ChainlinkRNG is RNG, VRFConsumerBaseV2Plus {
_;
}

modifier onlyBySortitionModule() {
require(sortitionModule == msg.sender, "SortitionModule only");
modifier onlyByConsumer() {
require(consumer == msg.sender, "Consumer only");
_;
}

Expand All @@ -57,7 +57,7 @@ contract ChainlinkRNG is RNG, VRFConsumerBaseV2Plus {

/// @dev Constructor, initializing the implementation to reduce attack surface.
/// @param _governor The Governor of the contract.
/// @param _sortitionModule The address of the SortitionModule contract.
/// @param _consumer The address that can request random numbers.
/// @param _vrfCoordinator The address of the VRFCoordinator contract.
/// @param _keyHash The gas lane key hash value - Defines the maximum gas price you are willing to pay for a request in wei (ID of the off-chain VRF job).
/// @param _subscriptionId The unique identifier of the subscription used for funding requests.
Expand All @@ -66,15 +66,15 @@ contract ChainlinkRNG is RNG, VRFConsumerBaseV2Plus {
/// @dev https://docs.chain.link/vrf/v2-5/subscription/get-a-random-number
constructor(
address _governor,
address _sortitionModule,
address _consumer,
address _vrfCoordinator,
bytes32 _keyHash,
uint256 _subscriptionId,
uint16 _requestConfirmations,
uint32 _callbackGasLimit
) VRFConsumerBaseV2Plus(_vrfCoordinator) {
governor = _governor;
sortitionModule = _sortitionModule;
consumer = _consumer;
keyHash = _keyHash;
subscriptionId = _subscriptionId;
requestConfirmations = _requestConfirmations;
Expand All @@ -91,10 +91,10 @@ contract ChainlinkRNG is RNG, VRFConsumerBaseV2Plus {
governor = _governor;
}

/// @dev Changes the sortition module of the contract.
/// @param _sortitionModule The new sortition module.
function changeSortitionModule(address _sortitionModule) external onlyByGovernor {
sortitionModule = _sortitionModule;
/// @dev Changes the consumer of the RNG.
/// @param _consumer The new consumer.
function changeConsumer(address _consumer) external onlyByGovernor {
consumer = _consumer;
}

/// @dev Changes the VRF Coordinator of the contract.
Expand Down Expand Up @@ -132,8 +132,8 @@ contract ChainlinkRNG is RNG, VRFConsumerBaseV2Plus {
// * State Modifiers * //
// ************************************* //

/// @dev Request a random number. SortitionModule only.
function requestRandomness(uint256 /*_block*/) external override onlyBySortitionModule {
/// @dev Request a random number. Consumer only.
function requestRandomness() external override onlyByConsumer {
// Will revert if subscription is not set and funded.
uint256 requestId = s_vrfCoordinator.requestRandomWords(
VRFV2PlusClient.RandomWordsRequest({
Expand Down Expand Up @@ -167,7 +167,7 @@ contract ChainlinkRNG is RNG, VRFConsumerBaseV2Plus {

/// @dev Return the random number.
/// @return randomNumber The random number or 0 if it is not ready or has not been requested.
function receiveRandomness(uint256 /*_block*/) external view override returns (uint256 randomNumber) {
function receiveRandomness() external view override returns (uint256 randomNumber) {
randomNumber = randomNumbers[lastRequestId];
}
}
13 changes: 13 additions & 0 deletions contracts/src/rng/IRNG.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// SPDX-License-Identifier: MIT

pragma solidity 0.8.24;

/// @title Random Number Generator interface
interface IRNG {
/// @dev Request a random number.
function requestRandomness() external;

/// @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);
}
Loading
Loading