Skip to content

Commit

Permalink
Fix event values (#2060)
Browse files Browse the repository at this point in the history 
* add withdrawal events to the tests

* add WETH accounting

* add better documentation

* add sending WETH to the Vault back to fix manual accounting function

* actually wrap the ETH to WETH before sending it to the Vault when manually fixing accounting

* add withdrawal event to manually fix accounting

* fix bugs and add tests with WETH accounting

* add test to verify correct deposits

* Gas changes to Native Staking's depositAll

---------

Co-authored-by: Nicholas Addison <[email protected]>
  • Loading branch information
@sparrowDom @naddison36
sparrowDom and naddison36 authored May 22, 2024
1 parent acad4b9 commit 638bf30
Show file tree
Hide file tree
Showing 5 changed files with 305 additions and 40 deletions.
59 changes: 55 additions & 4 deletions contracts/contracts/strategies/NativeStaking/NativeStakingSSVStrategy.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pragma solidity ^0.8.0;

import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "@openzeppelin/contracts/utils/math/Math.sol";

import { InitializableAbstractStrategy } from "../../utils/InitializableAbstractStrategy.sol";
import { IWETH9 } from "../../interfaces/IWETH9.sol";
Expand All @@ -18,6 +19,26 @@ struct ValidatorStakeData {
/// @title Native Staking SSV Strategy
/// @notice Strategy to deploy funds into DVT validators powered by the SSV Network
/// @author Origin Protocol Inc
/// @dev This contract handles WETH and ETH and in some operations interchanges between the two. Any WETH that
/// is on the contract across multiple blocks (and not just transitory within a transaction) is considered an
/// asset. Meaning deposits increase the balance of the asset and withdrawal decrease it. As opposed to all
/// our other strategies the WETH doesn't immediately get deposited into an underlying strategy and can be present
/// across multiple blocks waiting to be unwrapped to ETH and staked to validators. This separation of WETH and ETH is
/// required since the rewards (reward token) is also in ETH.
///
/// To simplify the accounting of WETH there is another difference in behavior compared to the other strategies.
/// To withdraw WETH asset - exit message is posted to validators and the ETH hits this contract with multiple days
/// delay. In order to simplify the WETH accounting upon detection of such an event the ValidatorAccountant
/// immediately wraps ETH to WETH and sends it to the Vault.
///
/// On the other hand any ETH on the contract (across multiple blocks) is there either:
/// - as a result of already accounted for consensus rewards
/// - as a result of not yet accounted for consensus rewards
/// - as a results of not yet accounted for full validator withdrawals (or validator slashes)
///
/// Even though the strategy assets and rewards are a very similar asset the consensus layer rewards and the
/// execution layer rewards are considered rewards and those are dripped to the Vault over a configurable time
/// interval and not immediately.
contract NativeStakingSSVStrategy is
ValidatorAccountant,
InitializableAbstractStrategy
Expand All @@ -33,8 +54,20 @@ contract NativeStakingSSVStrategy is
/// (rewards for arranging transactions in a way that benefits the validator).
address payable public immutable FEE_ACCUMULATOR_ADDRESS;

/// @dev This contract receives WETH as the deposit asset, but unlike other strategies doesn't immediately
/// deposit it to an underlying platform. Rather a special privilege account stakes it to the validators.
/// For that reason calling WETH.balanceOf(this) in a deposit function can contain WETH that has just been
/// deposited and also WETH that has previously been deposited. To keep a correct count we need to keep track
/// of WETH that has already been accounted for.
/// This value represents the amount of WETH balance of this contract that has already been accounted for by the
/// deposit events.
/// It is important to note that this variable is not concerned with WETH that is a result of full/partial
/// withdrawal of the validators. It is strictly concerned with WETH that has been deposited and is waiting to
/// be staked.
uint256 depositedWethAccountedFor;

// For future use
uint256[50] private __gap;
uint256[49] private __gap;

/// @param _baseConfig Base strategy config with platformAddress (ERC-4626 Vault contract), eg sfrxETH or sDAI,
/// and vaultAddress (OToken Vault contract), eg VaultProxy or OETHVaultProxy
Expand Down Expand Up @@ -126,6 +159,7 @@ contract NativeStakingSSVStrategy is
nonReentrant
{
require(_asset == WETH_TOKEN_ADDRESS, "Unsupported asset");
depositedWethAccountedFor += _amount;
_deposit(_asset, _amount);
}

Expand Down Expand Up @@ -154,8 +188,12 @@ contract NativeStakingSSVStrategy is
uint256 wethBalance = IERC20(WETH_TOKEN_ADDRESS).balanceOf(
address(this)
);
if (wethBalance > 0) {
_deposit(WETH_TOKEN_ADDRESS, wethBalance);
uint256 newWeth = wethBalance - depositedWethAccountedFor;

if (newWeth > 0) {
depositedWethAccountedFor = wethBalance;

_deposit(WETH_TOKEN_ADDRESS, newWeth);
}
}

Expand Down Expand Up @@ -262,7 +300,20 @@ contract NativeStakingSSVStrategy is
);
}

function wethWithdrawnToVault(uint256 _amount) internal override {
function _wethWithdrawnToVault(uint256 _amount) internal override {
emit Withdrawal(WETH_TOKEN_ADDRESS, address(0), _amount);
}

function _wethWithdrawnAndStaked(uint256 _amount) internal override {
/* In an ideal world we wouldn't need to reduce the deduction amount when the
* depositedWethAccountedFor is smaller than the _amount.
*
* The reason this is required is that a malicious actor could sent WETH direclty
* to this contract and that would circumvent the increase of depositedWethAccountedFor
* property. When the ETH would be staked the depositedWethAccountedFor amount could
* be deducted so much that it would be negative.
*/
uint256 deductAmount = Math.min(_amount, depositedWethAccountedFor);
depositedWethAccountedFor -= deductAmount;
}
}
37 changes: 29 additions & 8 deletions contracts/contracts/strategies/NativeStaking/ValidatorAccountant.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ abstract contract ValidatorAccountant is ValidatorRegistrator {

event AccountingManuallyFixed(
int256 validatorsDelta,
int256 consensusRewardsDelta
int256 consensusRewardsDelta,
uint256 wethToVault
);

/// @param _wethAddress Address of the Erc20 WETH Token contract
Expand Down Expand Up @@ -132,8 +133,8 @@ abstract contract ValidatorAccountant is ValidatorRegistrator {
IWETH9(WETH_TOKEN_ADDRESS).deposit{ value: wethToVault }();
// slither-disable-next-line unchecked-transfer
IWETH9(WETH_TOKEN_ADDRESS).transfer(VAULT_ADDRESS, wethToVault);
wethWithdrawnToVault(wethToVault);
_wethWithdrawnToVault(wethToVault);

emit AccountingFullyWithdrawnValidator(
fullyWithdrawnValidators,
activeDepositedValidators,
Expand Down Expand Up @@ -163,7 +164,7 @@ abstract contract ValidatorAccountant is ValidatorRegistrator {
IWETH9(WETH_TOKEN_ADDRESS).transfer(VAULT_ADDRESS, ethRemaining);
activeDepositedValidators -= 1;

wethWithdrawnToVault(ethRemaining);
_wethWithdrawnToVault(ethRemaining);

emit AccountingValidatorSlashed(
activeDepositedValidators,
Expand Down Expand Up @@ -194,9 +195,16 @@ abstract contract ValidatorAccountant is ValidatorRegistrator {
/// @notice Allow the Strategist to fix the accounting of this strategy and unpause.
/// @param _validatorsDelta adjust the active validators by up to plus three or minus three
/// @param _consensusRewardsDelta adjust the accounted for consensus rewards up or down
/// @param _ethToVaultAmount the amount of ETH that gets wrapped into WETH and sent to the Vault
/// @dev There is a case when a validator(s) gets slashed so much that the eth swept from
/// the beacon chain enters the fuse area and there are no consensus rewards on the contract
/// to "dip into"/use. To increase the amount of unaccounted ETH over the fuse end interval
/// we need to reduce the amount of active deposited validators and immediately send WETH
/// to the vault, so it doesn't interfere with further accounting.
function manuallyFixAccounting(
int256 _validatorsDelta,
int256 _consensusRewardsDelta
int256 _consensusRewardsDelta,
uint256 _ethToVaultAmount
) external onlyStrategist whenPaused {
require(
lastFixAccountingBlockNumber + MIN_FIX_ACCOUNTING_CADENCE <
Expand All @@ -217,17 +225,30 @@ abstract contract ValidatorAccountant is ValidatorRegistrator {
int256(consensusRewards) + _consensusRewardsDelta >= 0,
"invalid consensusRewardsDelta"
);
require(_ethToVaultAmount <= 32 ether * 3, "invalid wethToVaultAmount");

emit AccountingManuallyFixed(_validatorsDelta, _consensusRewardsDelta);
emit AccountingManuallyFixed(
_validatorsDelta,
_consensusRewardsDelta,
_ethToVaultAmount
);

activeDepositedValidators = uint256(
int256(activeDepositedValidators) + _validatorsDelta
);
consensusRewards = uint256(
int256(consensusRewards) + _consensusRewardsDelta
);

lastFixAccountingBlockNumber = block.number;
if (_ethToVaultAmount > 0) {
IWETH9(WETH_TOKEN_ADDRESS).deposit{ value: _ethToVaultAmount }();
// slither-disable-next-line unchecked-transfer
IWETH9(WETH_TOKEN_ADDRESS).transfer(
VAULT_ADDRESS,
_ethToVaultAmount
);
_wethWithdrawnToVault(_ethToVaultAmount);
}

// rerun the accounting to see if it has now been fixed.
// Do not pause the accounting on failure as it is already paused
Expand All @@ -242,5 +263,5 @@ abstract contract ValidatorAccountant is ValidatorRegistrator {
****************************************/

/// @dev allows for NativeStakingSSVStrategy contract to emit Withdrawal event
function wethWithdrawnToVault(uint256 _amount) internal virtual;
function _wethWithdrawnToVault(uint256 _amount) internal virtual;
}
8 changes: 8 additions & 0 deletions contracts/contracts/strategies/NativeStaking/ValidatorRegistrator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ abstract contract ValidatorRegistrator is Governable, Pausable {

// Convert required ETH from WETH
IWETH9(WETH_TOKEN_ADDRESS).withdraw(requiredETH);
_wethWithdrawnAndStaked(requiredETH);

uint256 validatorsLength = validators.length;
// For each validator
Expand Down Expand Up @@ -247,4 +248,11 @@ abstract contract ValidatorRegistrator is Governable, Pausable {
cluster
);
}

/***************************************
Abstract
****************************************/

/// @dev allows for NativeStakingSSVStrategy contract know how much WETH had been staked
function _wethWithdrawnAndStaked(uint256 _amount) internal virtual;
}
69 changes: 57 additions & 12 deletions contracts/test/behaviour/ssvStrategy.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,16 +127,23 @@ const shouldBehaveLikeAnSsvStrategy = (context) => {
});

describe("Validator operations", function () {
beforeEach(async () => {
const { weth, domen, nativeStakingSSVStrategy } = await context();
const depositToStrategy = async (amount) => {
const { weth, domen, nativeStakingSSVStrategy, oethVault, strategist } =
await context();

// Add 32 WETH to the strategy so it can be staked
await weth
.connect(domen)
.transfer(nativeStakingSSVStrategy.address, oethUnits("32"));
});
// Add 32 WETH to the strategy via a Vualt deposit
await weth.connect(domen).transfer(oethVault.address, amount);

return await oethVault
.connect(strategist)
.depositToStrategy(
nativeStakingSSVStrategy.address,
[weth.address],
[amount]
);
};

it("Should register and staked 32 ETH by validator registrator", async () => {
const registerAndStakeEth = async () => {
const {
addresses,
weth,
Expand All @@ -157,15 +164,15 @@ const shouldBehaveLikeAnSsvStrategy = (context) => {
ssvNetwork: addresses.SSVNetwork,
});

const stakeAmount = oethUnits("32");

await setERC20TokenBalance(
nativeStakingSSVStrategy.address,
ssv,
"1000",
hre
);

const stakeAmount = oethUnits("32");

// Register a new validator with the SSV Network
const regTx = await nativeStakingSSVStrategy
.connect(validatorRegistrator)
Expand All @@ -180,7 +187,7 @@ const shouldBehaveLikeAnSsvStrategy = (context) => {
.to.emit(nativeStakingSSVStrategy, "SSVValidatorRegistered")
.withArgs(testValidator.publicKey, testValidator.operatorIds);

// Stake 32 ETH to the new validator
// Stake stakeAmount ETH to the new validator
const stakeTx = await nativeStakingSSVStrategy
.connect(validatorRegistrator)
.stakeEth([
Expand All @@ -195,7 +202,7 @@ const shouldBehaveLikeAnSsvStrategy = (context) => {
.to.emit(nativeStakingSSVStrategy, "ETHStaked")
.withNamedArgs({
pubkey: testValidator.publicKey,
amount: stakeAmount,
amount: oethUnits("32"),
});

expect(await weth.balanceOf(nativeStakingSSVStrategy.address)).to.equal(
Expand All @@ -204,6 +211,43 @@ const shouldBehaveLikeAnSsvStrategy = (context) => {
"strategy WETH not decreased"
)
);
};

it("Should register and stake 32 ETH by validator registrator", async () => {
await depositToStrategy(oethUnits("32"));
await registerAndStakeEth();
});

it("Should emit correct values in deposit event", async () => {
const { weth, nativeStakingSSVStrategy } = await context();

await depositToStrategy(oethUnits("40"));
// at least 8 WETH has remained on the contract and a deposit all
// event should emit a correct amount
await registerAndStakeEth();

/* deposit to strategy calls depositAll on the strategy contract after sending the WETH
* to it. The event should contain only the amount of newly deposited WETH and not include
* the pre-exiting WETH.
*/
const tx = await depositToStrategy(parseEther("10"));

await expect(tx)
.to.emit(nativeStakingSSVStrategy, "Deposit")
.withArgs(weth.address, AddressZero, parseEther("10"));
});

it("Should register and stake 32 ETH even if half supplied by a 3rd party", async () => {
const { weth, domen, nativeStakingSSVStrategy } = await context();

await depositToStrategy(oethUnits("16"));
// A malicious actor is sending WETH directly to the native staking contract hoping to
// mess up the accounting.
await weth
.connect(domen)
.transfer(nativeStakingSSVStrategy.address, oethUnits("16"));

await registerAndStakeEth();
});

it("Should exit and remove validator by validator registrator", async () => {
Expand All @@ -215,6 +259,7 @@ const shouldBehaveLikeAnSsvStrategy = (context) => {
addresses,
testValidator,
} = await context();
await depositToStrategy(oethUnits("32"));

const { cluster } = await getClusterInfo({
ownerAddress: nativeStakingSSVStrategy.address,
Expand Down
Loading

0 comments on commit 638bf30

Please sign in to comment.