Ethena Labs - ciphermarco's results

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L35-L36 https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L190-L194 https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L225-L239

Vulnerability details

Impact

The StakedUSDe contract is vulnerable to manipulation by a malicious actor, leading to a permanent interruption of operations through a Denial-of-Service (DoS) attack. This vulnerability also impacts StakedUSDeV2 due to its inheritance of the StakedUSDe contract.

Proof of Concept

The contract acknowledges and attempts to mitigate the ERC-4626 donation attack, also known as inflation attack, by mandating that both _deposit and _withdraw functions validate the retention of at least 1 ether worth of shares within the contract.

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

  /// @notice Minimum non-zero shares amount to prevent donation attack
  uint256 private constant MIN_SHARES = 1 ether;

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

  /// @notice ensures a small non-zero amount of shares does not remain, exposing to donation attack
  function _checkMinShares() internal view {
    uint256 _totalSupply = totalSupply();
    if (_totalSupply > 0 && _totalSupply < MIN_SHARES) revert MinSharesViolation();
  }

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

  // @audit Called by `deposit` function
  function _deposit(address caller, address receiver, uint256 assets, uint256 shares)
    internal
    override
    nonReentrant
    notZero(assets)
    notZero(shares)
  {
    // ... SNIP ...
    _checkMinShares(); // <<<
  }

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

  // @audit Called by `redeem` function
  function _withdraw(address caller, address receiver, address _owner, uint256 assets, uint256 shares)
    internal
    override
    nonReentrant
    notZero(assets)
    notZero(shares)
  {
    // ... SNIP ...
    _checkMinShares(); // <<<
  }

However, this attempt to mitigate the inflation attack problem gives rise to new issues. The attacker can still manipulate the contract at a low cost but to render it inoperable instead of making a direct profit from the exploit.

Executable Proof of Concept

To integrate the testMinSharesViolationDoSAUDIT function, paste it within the test contract, specifically in the file /test/foundry/staking/StakedUSDe.t.sol.

// ... SNIP ...
contract StakedUSDeTest is Test, IERC20Events {
// ... SNIP ...
  function testMinSharesViolationDoSAUDIT() public {
    address attacker = address(0x1337);
    address victim = address(0xdeadbeef);

    // :: Attacker ::
    uint256 aAmount = 1_000_000 wei; // 0.000000000001 ether
    usdeToken.mint(attacker, aAmount);

    // Donates `1_000_000 wei` to the contract
    vm.startPrank(attacker);
    usdeToken.transfer(address(stakedUSDe), aAmount);
    vm.stopPrank();

    // :: Victim ::
    uint256 vTotalAmount = 1_000_000 ether;
    usdeToken.mint(victim, vTotalAmount);

    vm.startPrank(victim);
    usdeToken.approve(address(stakedUSDe), vTotalAmount);
    
    // Attempts `1 ether` amount deposit
    uint256 vCurAmount = 1 ether;
    vm.expectRevert(IStakedUSDe.MinSharesViolation.selector); // Revert
    stakedUSDe.deposit(vCurAmount, victim);

    // Attempts `10 ether` amount deposit
    vCurAmount = 10 ether;
    vm.expectRevert(IStakedUSDe.MinSharesViolation.selector); // Revert
    stakedUSDe.deposit(vCurAmount, victim);

    // Attempts `100 ether` amount deposit
    vCurAmount = 100 ether;
    vm.expectRevert(IStakedUSDe.MinSharesViolation.selector); // Revert
    stakedUSDe.deposit(vCurAmount, victim);

    // Attempts `1_000_000 ether` amount deposit
    vCurAmount = 1_000_000 ether;
    vm.expectRevert(IStakedUSDe.MinSharesViolation.selector); // Revert
    stakedUSDe.deposit(vCurAmount, victim);

    vm.stopPrank();
  }
// ... SNIP ...

Then you can execute it with the command:

$ forge test --match-test testMinSharesViolationDoSAUDIT

Root Cause

When the ERC-4626 deposit function is called, it internally calls previewDeposit(assets), which in turn triggers the _convertToShares(assets, Math.Rounding.Floor) method. This is how _convertToShares operates:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol#L229-L231

    function _convertToShares(uint256 assets, Math.Rounding rounding) internal view virtual returns (uint256) {
        return assets.mulDiv(totalSupply() + 10 ** _decimalsOffset(), totalAssets() + 1, rounding);
    }

After donating 1_000_000 wei (now returned by totalAssets()) to the contract, a deposit of, for example, 1_000_000 ether will lead to _convertToShares returning 999999000000999999 shares (< 1 ether). Consequently, this will prompt _checkMinShares to revert during the comparison of _totalSupply with MIN_SHARES (1 ether):

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

  /// @notice ensures a small non-zero amount of shares does not remain, exposing to donation attack
  function _checkMinShares() internal view {
    uint256 _totalSupply = totalSupply();
    if (_totalSupply > 0 && _totalSupply < MIN_SHARES) revert MinSharesViolation();
  }

Tools Used

Manual: code editor, Foundry's forge.

Recommended Mitigation Steps

1. Remove the _checkMinShares function.

2. Examine the existing measures implemented by the imported ERC4626 contract to mitigate the issue, and explore alternative compatible strategies for further mitigation if necessary.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol#L20-L47

 * [CAUTION]
 * ====
 * In empty (or nearly empty) ERC-4626 vaults, deposits are at high risk of being stolen through frontrunning
 * with a "donation" to the vault that inflates the price of a share. This is variously known as a donation or inflation
 * attack and is essentially a problem of slippage. Vault deployers can protect against this attack by making an initial
 * deposit of a non-trivial amount of the asset, such that price manipulation becomes infeasible. Withdrawals may
 * similarly be affected by slippage. Users can protect against this attack as well as unexpected slippage in general by
 * verifying the amount received is as expected, using a wrapper that performs these checks such as
 * https://github.com/fei-protocol/ERC4626#erc4626router-and-base[ERC4626Router].
 *
 * Since v4.9, this implementation uses virtual assets and shares to mitigate that risk. The `_decimalsOffset()`
 * corresponds to an offset in the decimal representation between the underlying asset's decimals and the vault
 * decimals. This offset also determines the rate of virtual shares to virtual assets in the vault, which itself
 * determines the initial exchange rate. While not fully preventing the attack, analysis shows that the default offset
 * (0) makes it non-profitable, as a result of the value being captured by the virtual shares (out of the attacker's
 * donation) matching the attacker's expected gains. With a larger offset, the attack becomes orders of magnitude more
 * expensive than it is profitable. More details about the underlying math can be found
 * xref:erc4626.adoc#inflation-attack[here].
 *
 * The drawback of this approach is that the virtual shares do capture (a very small) part of the value being accrued
 * to the vault. Also, if the vault experiences losses, the users try to exit the vault, the virtual shares and assets
 * will cause the first user to exit to experience reduced losses in detriment to the last users that will experience
 * bigger losses. Developers willing to revert back to the pre-v4.9 behavior just need to override the
 * `_convertToShares` and `_convertToAssets` functions.
 *
 * To learn more, check out our xref:ROOT:erc4626.adoc[ERC-4626 guide].
 * ====
 */

Assessed type

DoS