Ethena Labs - 0xpiken's results

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L225-L238

Vulnerability details

Impact

Full restricted staker can redeem their stUSDe regardless of their restriction.

Proof of Concept

• StakedUSDe.withdraw() and StakedUSDe.redeem() can be called by any one to redeem their shares stUSDes for assets USDes.
• Internal function _withdraw() will be called by StakedUSDe.withdraw() or StakedUSDe.redeem() to check if the caller is allowed to access these functions. Full restricted staker should be forbidden to redeem their stUSDe for USDe. However, this check will be bypassed if an unrestricted caller withdraws assets/redeems shares on behalf of the full restricted staker.

Copy the below test codes into StakedUSDe.blacklist.t.sol and run forge test --match-test test_fullBlacklist_bob_withdraw_on_behalf_of_alice_pass:

  function test_fullBlacklist_bob_withdraw_on_behalf_of_alice_pass() public {
    _mintApproveDeposit(alice, _amount, false);

    vm.prank(owner);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, alice);
    //@audit-info make sure alice is full restricted
    assertEq(stakedUSDe.hasRole(FULL_RESTRICTED_STAKER_ROLE, alice), true);

    uint256 balBefore = usdeToken.balanceOf(bob);

    vm.prank(alice);
    stakedUSDe.approve(bob, type(uint).max);
    vm.prank(bob);
    //@audit-info bob redeem _amount of stUSDe on behalf of alice, and send USDe to bob
    stakedUSDe.redeem(_amount, bob, alice);

    uint256 balAfter = usdeToken.balanceOf(bob);
    //@audit-info check if bob received _amount of USDe
    assertApproxEqAbs(_amount, balAfter - balBefore, 1);
  }

Tools Used

Manual review

Recommended Mitigation Steps

Modifies the function _withdraw(). It should also be reverted if owner is full restricted:

  function _withdraw(address caller, address receiver, address _owner, uint256 assets, uint256 shares)
    internal
    override
    nonReentrant
    notZero(assets)
    notZero(shares)
  {
-   if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver)) {
+   if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver) || hasRole(FULL_RESTRICTED_STAKER_ROLE, _owner)) {
      revert OperationNotAllowed();
    }

    super._withdraw(caller, receiver, _owner, assets, shares);
    _checkMinShares();
  }

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Caller who requested asset withdrawing or redeeming on behalf of owner is not be able to access asset when cooldown period ends.

Proof of Concept

StakedUSDeV2 defines an unstake cooldown period cooldownDuration.

• When cooldownDuration is zero, caller can call withdraw() or redeem() on behalf of owner if they have enough allowance approved by owner. The allowance will be reduced correspondingly if it is not set to type(uint).max.
• When cooldownDuration is not zero, caller can call cooldownAssets() or cooldownShares() on behalf of owner if they have enough allowance approved by owner. Similarly, the allowance will be reduced if it is not set to type(uint).max. Asset will be locked in silo contract and can only be withdrawed after cooldown period.

However, the caller can not withdraw asset when cooldown period ends because cooldowns was set wrongly. Only owner has right to withdraw asset from silo, which is unreasonable.

Copy the below test codes into StakedUSDeV2.cooldownEnabled.t.sol and run run forge test --match-test testBobRedeemSharesOnBehalfAlice:

  function testBobRedeemSharesOnBehalfAlice() public {
    uint256 amount = 100 ether;
    _mintApproveDeposit(alice, amount);

    assertEq(usdeToken.balanceOf(alice), 0);
    assertEq(usdeToken.balanceOf(address(stakedUSDe)), amount);
    assertEq(stakedUSDe.balanceOf(alice), amount);


    uint256 balOfBobBefore = usdeToken.balanceOf(bob);
    uint256 balOfAliceBefore = usdeToken.balanceOf(alice);
    //@audit-info Sets type(uint).max` as the allowance of bob over alice's tokens.
    vm.prank(alice);
    stakedUSDe.approve(bob, type(uint).max);
    //@audit-info Bob redeem amount of shares on behalf of alice
    vm.prank(bob);
    stakedUSDe.cooldownShares(amount, alice);

    skip(stakedUSDe.cooldownDuration() + 1);
    //@audit-info Bob withdraw redeemed asset and send it to himself after cooldown period ends
    vm.prank(bob);
    stakedUSDe.unstake(bob);

    //@audit-info Alice withdraw redeemed asset and send it to herself after cooldown period ends
    vm.prank(alice);
    stakedUSDe.unstake(alice);

    uint256 balOfBobAfter = usdeToken.balanceOf(bob);
    uint256 balOfAliceAfter = usdeToken.balanceOf(alice);
    //@audit-info bob received none of asset
    assertEq(balOfBobBefore, balOfBobAfter, "balance of Bob didn't change");
    //@audit-info However alice received amount of asset
    assertEq(balOfAliceBefore+amount, balOfAliceAfter, "Alice didn't receive asset");
  }

Tools Used

Manual review

Recommended Mitigation Steps

Asset redeemed / withdrawed should be assigned to caller instead of owner:

  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;
+   cooldowns[_msgSender()].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
+   cooldowns[_msgSender()].underlyingAmount += assets;

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

    return shares;
  }

  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;
+   cooldowns[_msgSender()].cooldownEnd = uint104(block.timestamp) + cooldownDuration;
+   cooldowns[_msgSender()].underlyingAmount += assets;

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

    return assets;
  }

Assessed type

Error

[01] L91 in transferInRewards() is unnecessory because getUnvestedAmount() is always zero:

  function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
    if (getUnvestedAmount() > 0) revert StillVesting();
-   uint256 newVestingAmount = amount + getUnvestedAmount();

-   vestingAmount = newVestingAmount;
+   vestingAmount = amount;
    lastDistributionTimestamp = block.timestamp;
    // transfer assets from rewarder to this contract
    IERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);

-   emit RewardsReceived(amount, newVestingAmount);
+   emit RewardsReceived(amount, amount);
  }

[02] Define MAX_COOLDOWN_DURATION as constant:

- uint24 public MAX_COOLDOWN_DURATION = 90 days;
+ uint24 public constant MAX_COOLDOWN_DURATION = 90 days;

[03] The number of calls to token.safeTransferFrom() in EthenaMinting#_transferCollateral() can be reduced by on because the last amount in iteration and the remaining balance (if not zero) will be transferred to the same address:

  function _transferCollateral(
    uint256 amount,
    address asset,
    address benefactor,
    address[] calldata addresses,
    uint256[] calldata ratios
  ) internal {
    // cannot mint using unsupported asset or native ETH even if it is supported for redemptions
    if (!_supportedAssets.contains(asset) || asset == NATIVE_TOKEN) revert UnsupportedAsset();
    IERC20 token = IERC20(asset);
    uint256 totalTransferred = 0;
-   for (uint256 i = 0; i < addresses.length; ++i) {
+   for (uint256 i = 0; i < addresses.length - 1; ++i) {
      uint256 amountToTransfer = (amount * ratios[i]) / 10_000;
      token.safeTransferFrom(benefactor, addresses[i], amountToTransfer);
      totalTransferred += amountToTransfer;
    }
    uint256 remainingBalance = amount - totalTransferred;
-   if (remainingBalance > 0) {
      token.safeTransferFrom(benefactor, addresses[addresses.length - 1], remainingBalance);
-   }
  }