code-423n4 / 2022-06-yieldy-findings

0 stars 0 forks source link

Warmup and cooldown periods do not enforce a full staking epoch #287

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L688-L693

Vulnerability details

Impact

The warmup and cooldown periods implemented in the Staking contract do not enforce a full epoch of staking.

Shortly before an epoch ends, one can stake USDC (staking token), have USDCy (reward tokens) locked up in the warmup period and immediately call unstake.

Immediately after the next epoch begins, the staking tokens (USDC) can be withdrawn. The user did not have to wait for a full warm-up and cooldown period.

No additional rewards are received as rewards are issued in a 1 period lagging fashion, however, the user who staked his funds and unstaked immediately after, will lose out on those rewards (the staked capital was working to accrue rewards on Tokemak).

Proof of Concept

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L688-L693

The following test case demonstrates how one can stake and unstake regardless of the warmup and cooldown period:

it("User can stake and unstake within very small time frame regardless of warmup and cooldown period", async () => {
  const { staker1 } = await getNamedAccounts();

  const transferAmount = BigNumber.from("10000");
  await stakingToken.transfer(staker1, transferAmount);

  const staker1Signer = accounts.find((account) => account.address === staker1);
  await staking.setWarmUpPeriod(1);
  await staking.setCoolDownPeriod(1);
  const stakingStaker1 = staking.connect(staker1Signer as Signer);

  const stakingAmount = transferAmount;
  const stakingTokenStaker1 = stakingToken.connect(staker1Signer as Signer);
  await stakingTokenStaker1.approve(staking.address, stakingAmount);

  /**
   * Epoch 1
   */

  // Stake shortly before current epoch ends
  await stakingStaker1.functions["stake(uint256)"](stakingAmount);

  let warmUpInfo = await staking.warmUpInfo(staker1);
  expect(warmUpInfo.amount).to.equal(stakingAmount);

  await rewardToken
    .connect(staker1Signer as Signer)
    .approve(staking.address, stakingAmount);

  // Unstake immediately after staking and shortly before current epoch ends
  await stakingStaker1.unstake(stakingAmount, false);

  let coolDownInfo = await staking.coolDownInfo(staker1);

  // Staker1 is in cooldown which lasts until epoch 2 starts
  expect(coolDownInfo.amount).to.equal(stakingAmount);

  await mineBlocksToNextCycle();

  /**
   * Epoch 2
   */

  // need to rebase to increase epoch number
  await stakingStaker1.rebase();
  await stakingStaker1.sendWithdrawalRequests();

  await network.provider.request({
    method: "hardhat_impersonateAccount",
    params: [constants.TOKE_OWNER],
  });
  const tokeSigner = await ethers.getSigner(constants.TOKE_OWNER);
  const tokeManagerOwner = tokeManager.connect(tokeSigner);
  await tokeManagerOwner.completeRollover(constants.LATEST_CLAIMABLE_HASH);

  // Withdraw staking tokens
  await stakingStaker1.claimWithdraw(staker1);

  // has stakingBalance after withdrawal
  let stakingTokenBalance = await stakingToken.balanceOf(staker1);

  expect(stakingTokenBalance).eq(stakingAmount);
});

Tools Used

Manual review

Recommended mitigation steps

Consider implementing the warmup period in a way that the warmup period is bound to the amount that was staked at that time by the account.

toshiSat commented 2 years ago

sponsor disputed: this is by design. currently we have the cooldown set to two epochs to prevent users from claiming right after the epoch rolls over. We want users to gain rewards in the warmup contract as well as not gaining rewards from the cooldown contract.