Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

IPC-361: Pre-release and minor fixes #271

Merged
merged 10 commits into from
Nov 2, 2023
10 changes: 4 additions & 6 deletions src/gateway/GatewayRouterFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -232,10 +232,8 @@ contract GatewayRouterFacet is GatewayActorModifiers {
revert CheckpointNotCreated();
}

bytes32 checkpointHash = checkpointInfo.hash;

// slither-disable-next-line unused-return
(address recoveredSignatory, ECDSA.RecoverError err, ) = ECDSA.tryRecover(checkpointHash, signature);
(address recoveredSignatory, ECDSA.RecoverError err, ) = ECDSA.tryRecover(checkpointInfo.hash, signature);
if (err != ECDSA.RecoverError.NoError) {
revert InvalidSignature();
}
Expand Down Expand Up @@ -273,13 +271,13 @@ contract GatewayRouterFacet is GatewayActorModifiers {
}
emit QuorumReached({
height: height,
checkpoint: checkpointHash,
checkpoint: checkpointInfo.hash,
quorumWeight: checkpointInfo.currentWeight
});
} else {
emit QuorumWeightUpdated({
height: height,
checkpoint: checkpointHash,
checkpoint: checkpointInfo.hash,
newWeight: checkpointInfo.currentWeight
});
}
Expand Down Expand Up @@ -316,7 +314,7 @@ contract GatewayRouterFacet is GatewayActorModifiers {
if (!ok) {
revert FailedAddIncompleteCheckpoint();
}

CheckpointInfo memory info = CheckpointInfo({
hash: checkpoint.toHash(),
rootHash: membershipRootHash,
Expand Down
3 changes: 3 additions & 0 deletions src/interfaces/ISubnetActor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ interface ISubnetActor {
/// Method that allows to pre-fund an address in the subnet before it bootstraps.
function preFund() external payable;

/// Method that allows to recover initial balance for an address from a subnet that hasn't bootstrapped yet.
function preRelease(uint256 amount) external;

/// Method that allows a validator to unstake their collateral from a subnet
function unstake(uint256 amount) external;

Expand Down
13 changes: 8 additions & 5 deletions src/lib/LibGateway.sol
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,11 @@ library LibGateway {
/// @notice returns the bottom-up checkpoint and its info at the target epoch
function getBottomUpCheckpointWithInfo(
uint64 epoch
) internal view returns (bool exists, BottomUpCheckpoint storage checkpoint, CheckpointInfo storage checkpointInfo) {
)
internal
view
returns (bool exists, BottomUpCheckpoint storage checkpoint, CheckpointInfo storage checkpointInfo)
{
GatewayActorStorage storage s = LibGatewayActorStorage.appStorage();

checkpoint = s.bottomUpCheckpoints[epoch];
Expand All @@ -53,16 +57,15 @@ library LibGateway {
}

/// @notice checks if the bottom-up checkpoint already exists at the target epoch
function bottomUpCheckpointExists(
uint64 epoch
) internal view returns (bool) {
function bottomUpCheckpointExists(uint64 epoch) internal view returns (bool) {
GatewayActorStorage storage s = LibGatewayActorStorage.appStorage();
return s.bottomUpCheckpoints[epoch].blockHeight != 0;
}

/// @notice stores checkpoint and its info to storage.
function storeBottomUpCheckpointWithInfo(
BottomUpCheckpoint memory checkpoint, CheckpointInfo memory checkpointInfo
BottomUpCheckpoint memory checkpoint,
CheckpointInfo memory checkpointInfo
) internal {
GatewayActorStorage storage s = LibGatewayActorStorage.appStorage();

Expand Down
3 changes: 2 additions & 1 deletion src/lib/LibStaking.sol
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,8 @@ library LibStaking {
}

/// @notice Confirm the withdraw directly without going through the confirmation process
/// and releasing from the gateway.
/// @dev only use for non-bootstrapped subnets
function withdrawWithConfirm(address validator, uint256 amount) internal {
SubnetActorStorage storage s = LibSubnetActorStorage.appStorage();

Expand All @@ -438,7 +440,6 @@ library LibStaking {
s.validatorSet.confirmWithdraw(validator, amount);

// release stake from gateway and transfer to user
IGateway(s.ipcGatewayAddr).releaseStake(amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's see if I understand this: the stake is added to the gateway in one go when the subnet registers itself, right? So it makes sense that before that, this should not be trying to release anything.

Strange we haven't caught any issues with this 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know. I feel that there were a bunch of tests that were commented so we lost track of this. But I included a few to catch it. Good that you raised this issue, as it already uncovered this bug :)

payable(validator).transfer(amount);
}

Expand Down
56 changes: 54 additions & 2 deletions src/subnet/SubnetActorManagerFacet.sol
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: MIT OR Apache-2.0
pragma solidity 0.8.19;

import {SubnetAlreadyBootstrapped, NotEnoughFunds, CollateralIsZero, CannotReleaseZero, NotOwnerOfPublicKey, EmptyAddress, NotEnoughBalanceForRewards, NotEnoughCollateral, NotValidator, NotAllValidatorsHaveLeft, NotStakedBefore, InvalidSignatureErr, InvalidCheckpointEpoch, InvalidCheckpointMessagesHash, InvalidPublicKeyLength} from "../errors/IPCErrors.sol";
import {SubnetAlreadyBootstrapped, NotEnoughFunds, CollateralIsZero, CannotReleaseZero, NotOwnerOfPublicKey, EmptyAddress, NotEnoughBalance, NotEnoughBalanceForRewards, NotEnoughCollateral, NotValidator, NotAllValidatorsHaveLeft, NotStakedBefore, InvalidSignatureErr, InvalidCheckpointEpoch, InvalidCheckpointMessagesHash, InvalidPublicKeyLength} from "../errors/IPCErrors.sol";
import {IGateway} from "../interfaces/IGateway.sol";
import {ISubnetActor} from "../interfaces/ISubnetActor.sol";
import {BottomUpCheckpoint, CrossMsg} from "../structs/Checkpoint.sol";
Expand Down Expand Up @@ -109,6 +109,31 @@
s.genesisCircSupply += msg.value;
}

/// @notice method to remove funds from the initial balance of a subnet.
/// @dev This method can be used by users looking to recover part of their
/// initial balance before the subnet bootstraps.
function preRelease(uint256 amount) external nonReentrant {
if (amount == 0) {
revert NotEnoughFunds();
}

if (s.bootstrapped) {
revert SubnetAlreadyBootstrapped();
}

if (s.genesisBalance[msg.sender] == 0) {
adlrocha marked this conversation as resolved.
Show resolved Hide resolved
revert NotEnoughBalance();
}

s.genesisBalance[msg.sender] -= amount;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this can't go negative, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It reverts on underflow/overflow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are uint256, they can't be negative (they could theoretically underflow though). However, if preFund and preRelease don't have bugs this shouldn't happen.

In spite of this, do you think we should add a sanity-check just to be safe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, okay, I recommended the balance < amount => revert fix, but if it would revert even if you try an underflow, then it's okay, what matters is there is no way get more out.

s.genesisCircSupply -= amount;

if (s.genesisBalance[msg.sender] == 0) {
rmAddressFromBalanceKey(msg.sender);
payable(msg.sender).sendValue(amount);
adlrocha marked this conversation as resolved.
Show resolved Hide resolved
}
}

/// @notice method that allows a validator to join the subnet
/// @param publicKey The off-chain 65 byte public key that should be associated with the validator
function join(bytes calldata publicKey) external payable nonReentrant notKilled {
Expand Down Expand Up @@ -196,7 +221,9 @@
}

/// @notice method that allows a validator to leave the subnet
function leave() external notKilled {
/// @dev it also return the validators initial balance if the
/// subnet was not yet bootstrapped.
function leave() external notKilled nonReentrant {
// remove bootstrap nodes added by this validator

uint256 amount = LibStaking.totalValidatorCollateral(msg.sender);
Expand All @@ -210,6 +237,15 @@

if (!s.bootstrapped) {
LibStaking.withdrawWithConfirm(msg.sender, amount);

// check if the validator had some initial balance and return it if not bootstrapped
uint256 genesisBalance = s.genesisBalance[msg.sender];
if (genesisBalance != 0) {
s.genesisBalance[msg.sender] == 0;
s.genesisCircSupply -= genesisBalance;
rmAddressFromBalanceKey(msg.sender);
payable(msg.sender).sendValue(genesisBalance);
}
return;
}
LibStaking.withdraw(msg.sender, amount);
Expand Down Expand Up @@ -319,4 +355,20 @@
bytes32 hashed = keccak256(publicKey[1:]);
return address(uint160(uint256(hashed)));
}

/// @notice Removes an address from the initial balance keys
function rmAddressFromBalanceKey(address addr) internal {
uint length = s.genesisBalanceKeys.length;

Check warning on line 361 in src/subnet/SubnetActorManagerFacet.sol

View workflow job for this annotation

GitHub Actions / Solhint check

Rule is set with explicit type [var/s: uint]
for (uint i = 0; i < length; ) {

Check warning on line 362 in src/subnet/SubnetActorManagerFacet.sol

View workflow job for this annotation

GitHub Actions / Solhint check

Rule is set with explicit type [var/s: uint]
if (s.genesisBalanceKeys[i] == addr) {
s.genesisBalanceKeys[i] = s.genesisBalanceKeys[length - 1];
s.genesisBalanceKeys.pop();
// exit after removing the key
break;
}
unchecked {
++i;
}
}
}
}
66 changes: 64 additions & 2 deletions test/SubnetActorDiamondReal.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -451,9 +451,10 @@ contract SubnetActorDiamondRealTest is Test {
);
}

function testSubnetActorDiamondReal_PreFund_works() public {
function testSubnetActorDiamondReal_PreFundRelease_works() public {
(address validator1, bytes memory publicKey1) = TestUtils.deriveValidatorAddress(100);
address preFunder = address(102);
address preReleaser = address(103);

// total collateral in the gateway
uint256 collateral = 0;
Expand All @@ -463,6 +464,21 @@ contract SubnetActorDiamondRealTest is Test {
require(!saGetter.isWaitingValidator(validator1), "waiting validator1");

// ======== Step. Join ======

// pre-fund and pre-release from same address
vm.startPrank(preReleaser);
vm.deal(preReleaser, 2 * fundAmount);
saManager.preFund{value: 2 * fundAmount}();
require(saGetter.genesisCircSupply() == 2 * fundAmount, "genesis circ supply not correct");
saManager.preRelease(fundAmount);
require(saGetter.genesisCircSupply() == fundAmount, "genesis circ supply not correct");
(address[] memory genesisAddrs, ) = saGetter.genesisBalances();
require(genesisAddrs.length == 1, "not one genesis addresses");
saManager.preRelease(fundAmount);
(genesisAddrs, ) = saGetter.genesisBalances();
require(saGetter.genesisCircSupply() == 0, "genesis circ supply not correct");
require(genesisAddrs.length == 0, "not zero genesis addresses");

// pre-fund from validator and from pre-funder
vm.startPrank(validator1);
vm.deal(validator1, fundAmount);
Expand All @@ -484,7 +500,7 @@ contract SubnetActorDiamondRealTest is Test {
);

require(saGetter.genesisCircSupply() == 2 * fundAmount, "genesis circ supply not correct");
(address[] memory genesisAddrs, ) = saGetter.genesisBalances();
(genesisAddrs, ) = saGetter.genesisBalances();
require(genesisAddrs.length == 2, "not two genesis addresses");

// collateral confirmed immediately and network boostrapped
Expand All @@ -502,11 +518,57 @@ contract SubnetActorDiamondRealTest is Test {
require(nextConfigNum == LibStaking.INITIAL_CONFIGURATION_NUMBER, "next config num not 1");
require(startConfigNum == LibStaking.INITIAL_CONFIGURATION_NUMBER, "start config num not 1");

// pre-fund not allowed with bootstrapped subnet
vm.startPrank(preFunder);
vm.expectRevert(SubnetAlreadyBootstrapped.selector);
vm.deal(preFunder, fundAmount);
saManager.preFund{value: fundAmount}();

vm.stopPrank();
}

function testSubnetActorDiamondReal_PreFundAndLeave_works() public {
(address validator1, bytes memory publicKey1) = TestUtils.deriveValidatorAddress(100);

// total collateral in the gateway
uint256 collateral = DEFAULT_MIN_VALIDATOR_STAKE - 100;
uint256 fundAmount = 100;

// pre-fund from validator
vm.startPrank(validator1);
vm.deal(validator1, fundAmount);
saManager.preFund{value: fundAmount}();

// initial validator joins but doesn't bootstrap the subnet
vm.deal(validator1, collateral);
vm.startPrank(validator1);
saManager.join{value: collateral}(publicKey1);
require(
address(saDiamond).balance == collateral + fundAmount,
"subnet balance is incorrect after validator1 joining"
);
require(saGetter.genesisCircSupply() == fundAmount, "genesis circ supply not correct");
(address[] memory genesisAddrs, ) = saGetter.genesisBalances();
require(genesisAddrs.length == 1, "not one genesis addresses");

// Leave should return the collateral and the initial balance
saManager.leave();
require(address(saDiamond).balance == 0, "subnet balance is incorrect after validator1 leaving");
require(saGetter.genesisCircSupply() == 0, "genesis circ supply not zero");
(genesisAddrs, ) = saGetter.genesisBalances();
require(genesisAddrs.length == 0, "not zero genesis addresses");

// initial validator joins to bootstrap the subnet
vm.deal(validator1, DEFAULT_MIN_VALIDATOR_STAKE);

vm.startPrank(validator1);
saManager.join{value: DEFAULT_MIN_VALIDATOR_STAKE}(publicKey1);

// pre-release not allowed with bootstrapped subnet
vm.startPrank(validator1);
vm.expectRevert(SubnetAlreadyBootstrapped.selector);
saManager.preRelease(fundAmount);

vm.stopPrank();
}
}
Loading