StakedUSDe.sol violates ERC4626 specification

[c4-submissions]

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

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Checks on SOFT_RESTRICTED_STAKER_ROLE are already in place to prevent minting.

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)

[fatherGoose1]

Downgrade to QA. While the report correctly identifies lack of compliance with ERC standard, it does not impose risk to the Ethena protocol. There is an ongoing discussion amongst judges on the severity of non-ERC compliance, and I rule that this does not deserve medium severity.

[c4-judge]

fatherGoose1 marked the issue as grade-b