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.

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

Enable async withdrawals on superOETHb #2275

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions contracts/contracts/interfaces/IVault.sol
Original file line number Diff line number Diff line change
Expand Up @@ -264,4 +264,8 @@ interface IVault {
external
view
returns (bool);

function withdrawalClaimDelay() external view returns (uint256);

function setWithdrawalClaimDelay(uint256 newDelay) external;
}
33 changes: 0 additions & 33 deletions contracts/contracts/vault/OETHBaseVaultCore.sol
Original file line number Diff line number Diff line change
Expand Up @@ -77,37 +77,4 @@ contract OETHBaseVaultCore is OETHVaultCore {

super._redeem(_amount, _minimumUnitAmount);
}

// @inheritdoc OETHVaultCore
// solhint-disable-next-line no-unused-vars
function requestWithdrawal(uint256 _amount)
external
virtual
override
returns (uint256, uint256)
{
revert("Async withdrawals disabled");
}

// @inheritdoc OETHVaultCore
// solhint-disable-next-line no-unused-vars
function claimWithdrawal(uint256 _requestId)
external
virtual
override
returns (uint256)
{
revert("Async withdrawals disabled");
}

// @inheritdoc OETHVaultCore
// solhint-disable-next-line no-unused-vars
function claimWithdrawals(uint256[] memory _requestIds)
external
virtual
override
returns (uint256[] memory, uint256)
{
revert("Async withdrawals disabled");
}
}
9 changes: 7 additions & 2 deletions contracts/contracts/vault/OETHVaultCore.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ contract OETHVaultCore is VaultCore {
using SafeERC20 for IERC20;
using StableMath for uint256;

uint256 public constant CLAIM_DELAY = 10 minutes;
address public immutable weth;
uint256 public wethAssetIndex;

Expand Down Expand Up @@ -172,6 +171,8 @@ contract OETHVaultCore is VaultCore {
nonReentrant
returns (uint256 requestId, uint256 queued)
{
require(withdrawalClaimDelay > 0, "Async withdrawals not enabled");

// The check that the requester has enough OETH is done in to later burn call

requestId = withdrawalQueueMetadata.nextWithdrawalIndex;
Expand Down Expand Up @@ -287,12 +288,16 @@ contract OETHVaultCore is VaultCore {
internal
returns (uint256 amount)
{
// Note: When `withdrawalClaimDelay` is set to 0, users can
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we will want to eventualy change the request.timestamp to become an end stamp rather than a start time stamp.

The current code has three awkward things:

  • If we change the claim duration, it retroactively changes the end date of inflight requests.
  • If we disable async claims by setting it to zero, it actually allows instant claims of all inflight claims.
  • If someone wrapped the governance execute() of a zero set, then they could flash loan a mint and a redeem in the same block.

However, our current upgrade on OETH will be atomically setting a value for this in the upgrade, and we don't have to use the zero value for now.

Given that the current zero value is dangerous, how about we also have the require nonzero in the claim? In this way we never accidentally give someone the ability to flashloan an 1:1 mint/redeem. We could then remove it at some future date once we've changed the code to make it safe.

// no longer place withdrawal requests. However, any existing
// unclaimed request would become immediately claimable.

// Load the structs from storage into memory
WithdrawalRequest memory request = withdrawalRequests[requestId];
WithdrawalQueueMetadata memory queue = withdrawalQueueMetadata;

require(
request.timestamp + CLAIM_DELAY <= block.timestamp,
request.timestamp + withdrawalClaimDelay <= block.timestamp,
"Claim delay not met"
);
// If there isn't enough reserved liquidity in the queue to claim
Expand Down
14 changes: 14 additions & 0 deletions contracts/contracts/vault/VaultAdmin.sol
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,20 @@ contract VaultAdmin is VaultStorage {
emit DripperChanged(_dripper);
}

/**
* @notice Changes the async withdrawal claim period for OETH & superOETHb
* @param _delay Delay period (should be between 10 mins to 7 days).
* Set to 0 to disable async withdrawals
*/
function setWithdrawalClaimDelay(uint256 _delay) external onlyGovernor {
require(
_delay == 0 || (_delay >= 10 minutes && _delay <= 7 days),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes sense to set the max delay to something that's greater than the max validator withdraw time. Perhaps 10 days? (At least one LST has a 15 day timed withdraw)

"Invalid claim delay period"
);
emit WithdrawalClaimDelayUpdated(_delay);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We usually like doing events last, when possible.

withdrawalClaimDelay = _delay;
}

/***************************************
Swaps
****************************************/
Expand Down
11 changes: 10 additions & 1 deletion contracts/contracts/vault/VaultStorage.sol
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ contract VaultStorage is Initializable, Governable {
uint256 _amount
);
event WithdrawalClaimable(uint256 _claimable, uint256 _newClaimable);
event WithdrawalClaimDelayUpdated(uint256 _newDelay);

// Assets supported by the Vault, i.e. Stablecoins
enum UnitConversion {
Expand Down Expand Up @@ -236,8 +237,16 @@ contract VaultStorage is Initializable, Governable {
/// @notice Mapping of withdrawal request indices to the user withdrawal request data
mapping(uint256 => WithdrawalRequest) public withdrawalRequests;

/// @notice Sets a minimum delay that is required to elapse between
/// requesting async withdrawals and claiming the request.
/// When set to 0 async withdrawals are disabled.
// slither-disable-start constable-states
// slither-disable-next-line uninitialized-state
shahthepro marked this conversation as resolved.
Show resolved Hide resolved
uint256 public withdrawalClaimDelay;
// slither-disable-end constable-states

// For future use
uint256[45] private __gap;
uint256[44] private __gap;

/**
* @notice set the implementation for the admin, this needs to be in a base class else we cannot set it
Expand Down
45 changes: 45 additions & 0 deletions contracts/deploy/base/018_async_withdrawals.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
const { deployOnBaseWithGuardian } = require("../../utils/deploy-l2");
const { deployWithConfirmation } = require("../../utils/deploy");
const addresses = require("../../utils/addresses");

module.exports = deployOnBaseWithGuardian(
{
deployName: "018_async_withdrawals",
},
async ({ ethers }) => {
const cOETHbVaultProxy = await ethers.getContract("OETHBaseVaultProxy");
const cOETHbVault = await ethers.getContractAt(
"IVault",
cOETHbVaultProxy.address
);

// Deploy new implementation
const dOETHbVaultCore = await deployWithConfirmation("OETHBaseVaultCore", [
addresses.base.WETH,
]);
const dOETHbVaultAdmin = await deployWithConfirmation("OETHBaseVaultAdmin");

return {
actions: [
{
// 1. Upgrade VaultCore
contract: cOETHbVaultProxy,
signature: "upgradeTo(address)",
args: [dOETHbVaultCore.address],
},
{
// 2. Upgrade VaultAdmin
contract: cOETHbVault,
signature: "setAdminImpl(address)",
args: [dOETHbVaultAdmin.address],
},
{
// 3. Set async claim delay to 1 day
contract: cOETHbVault,
signature: "setWithdrawalClaimDelay(uint256)",
args: [24 * 60 * 60], // 1d
},
],
};
}
);
5 changes: 5 additions & 0 deletions contracts/deploy/deployActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,11 @@ const configureOETHVault = async (isSimpleOETH) => {
.connect(sGovernor)
.setAutoAllocateThreshold(ethers.utils.parseUnits("5", 18))
);

// Set withdrawal claim delay to 10m
await withConfirmation(
cVault.connect(sGovernor).setWithdrawalClaimDelay(10 * 60)
);
};

const deployOUSDHarvester = async (ousdDripper) => {
Expand Down
55 changes: 55 additions & 0 deletions contracts/deploy/mainnet/108_vault_upgrade.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
const addresses = require("../../utils/addresses");
const { deploymentWithGovernanceProposal } = require("../../utils/deploy");

module.exports = deploymentWithGovernanceProposal(
{
deployName: "108_vault_upgrade",
forceDeploy: false,
//forceSkip: true,
reduceQueueTime: true,
deployerIsProposer: false,
proposalId: "",
},
async ({ deployWithConfirmation }) => {
// Deployer Actions
// ----------------

// 1. Deploy new OETH Vault Core and Admin implementations
const dVaultCore = await deployWithConfirmation("OETHVaultCore", [
addresses.mainnet.WETH,
]);
const dVaultAdmin = await deployWithConfirmation("OETHVaultAdmin", [
addresses.mainnet.WETH,
]);

// 2. Connect to the OETH Vault as its governor via the proxy
const cVaultProxy = await ethers.getContract("OETHVaultProxy");
const cVault = await ethers.getContractAt("IVault", cVaultProxy.address);

// Governance Actions
// ----------------
return {
name: "Upgrade OETH Vault",
actions: [
// 1. Upgrade the OETH Vault proxy to the new core vault implementation
{
contract: cVaultProxy,
signature: "upgradeTo(address)",
args: [dVaultCore.address],
},
// 2. Set OETH Vault proxy to the new admin vault implementation
{
contract: cVault,
signature: "setAdminImpl(address)",
args: [dVaultAdmin.address],
},
{
// 3. Set async claim delay to 10 minutes
contract: cVault,
signature: "setWithdrawalClaimDelay(uint256)",
args: [10 * 60], // 10 mins
},
],
};
}
);
1 change: 1 addition & 0 deletions contracts/hardhat.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ module.exports = {
mnemonic,
},
blockGasLimit: 1000000000,
allowUnlimitedContractSize: true,
chainId,
...(isArbitrumFork
? { tags: ["arbitrumOne"] }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,19 @@ const {
createFixtureLoader,
nodeRevert,
nodeSnapshot,
} = require("../_fixture");
} = require("../../_fixture");

const addresses = require("../../utils/addresses");
const { defaultBaseFixture } = require("../_fixture-base");
const addresses = require("../../../utils/addresses");
const { defaultBaseFixture } = require("../../_fixture-base");
const { expect } = require("chai");
const { oethUnits } = require("../helpers");
const { oethUnits } = require("../../helpers");
const ethers = require("ethers");
const { impersonateAndFund } = require("../../utils/signers");
const { impersonateAndFund } = require("../../../utils/signers");
//const { formatUnits } = ethers.utils;
const { BigNumber } = ethers;

const baseFixture = createFixtureLoader(defaultBaseFixture);
const { setERC20TokenBalance } = require("../_fund");
const { setERC20TokenBalance } = require("../../_fund");
const futureEpoch = 1924064072;

describe("ForkTest: Aerodrome AMO Strategy empty pool setup (Base)", async function () {
Expand Down Expand Up @@ -270,6 +270,7 @@ describe("ForkTest: Aerodrome AMO Strategy (Base)", async function () {
rafael,
aeroSwapRouter,
aeroNftManager,
harvester,
quoter;

beforeEach(async () => {
Expand All @@ -287,6 +288,7 @@ describe("ForkTest: Aerodrome AMO Strategy (Base)", async function () {
oethbVaultSigner = await impersonateAndFund(oethbVault.address);
gauge = fixture.aeroClGauge;
quoter = fixture.quoter;
harvester = fixture.harvester;

await setup();
await weth
Expand Down Expand Up @@ -369,7 +371,7 @@ describe("ForkTest: Aerodrome AMO Strategy (Base)", async function () {

// correct harvester set
expect(await aerodromeAmoStrategy.harvesterAddress()).to.equal(
await strategist.getAddress()
harvester.address
);

await assetLpStakedInGauge();
Expand Down Expand Up @@ -471,18 +473,16 @@ describe("ForkTest: Aerodrome AMO Strategy (Base)", async function () {

describe("Harvest rewards", function () {
it("Should be able to collect reward tokens", async () => {
const strategistAddr = await strategist.getAddress();

await setERC20TokenBalance(
aerodromeAmoStrategy.address,
aero,
"1337",
hre
);
const aeroBalanceBefore = await aero.balanceOf(strategistAddr);
await aerodromeAmoStrategy.connect(strategist).collectRewardTokens();
const aeroBalanceBefore = await aero.balanceOf(strategist.address);
await harvester.connect(strategist).harvest();

const aeroBalancediff = (await aero.balanceOf(strategistAddr)).sub(
const aeroBalancediff = (await aero.balanceOf(strategist.address)).sub(
aeroBalanceBefore
);

Expand Down
Loading
Loading