Bypassing `FULL_RESTRICTED_STAKER_ROLE` Restrictions with `_spendAllowance()`

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L217-L238 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC4626.sol#L267-L269 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L297-L315 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L284-L295

Vulnerability details

Impact

Users assigned the FULL_RESTRICTED_STAKER_ROLE, intended to be restricted from certain actions, can bypass these restrictions. They can use the _spendAllowance function (as inherited from ERC20.sol via ERC4626.sol) to grant allowances, potentially leading to unintended asset transfers, undermining the security measures put in place, and eroding trust in the system.

Evidently, StakedUSDeV2.cooldownAssets and StakedUSDeV2.cooldownShares that has been commented below:

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L94 https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L110

  /// @param owner address to redeem and start cooldown, owner must allowed caller to perform this action

facilitates an owner of stUSDe to allow a caller to invoke StakedUSDe._withdraw as _msgSender():

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L103 https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDeV2.sol#L119

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

Proof of Concept

Here's the scenario:

1. The protocol assign FULL_RESTRICTED_STAKER_ROLE to a user after they've staked their assets.
2. Despite the restriction, the user calls _approve() to grant a full allowance to another address.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L284-L295

    function _approve(address owner, address spender, uint256 value, bool emitEvent) internal virtual {
        if (owner == address(0)) {
            revert ERC20InvalidApprover(address(0));
        }
        if (spender == address(0)) {
            revert ERC20InvalidSpender(address(0));
        }
        _allowances[owner][spender] = value;
        if (emitEvent) {
            emit Approval(owner, spender, value);
        }
    }

3. The receiving address (caller) can now act on behalf of the restricted user, essentially bypassing the intended restrictions (because the if block does not check on the owner, just the caller and the receiver) when super._withdraw() is invoked from StakedUSDe._withdraw().

https://github.com/code-423n4/2023-10-ethena/blob/main/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();
  }

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

    function _withdraw(
        address caller,
        address receiver,
        address owner,
        uint256 assets,
        uint256 shares
    ) internal virtual {
        if (caller != owner) {
            _spendAllowance(owner, caller, shares);
        }

        _burn(owner, shares);
        SafeERC20.safeTransfer(_asset, receiver, assets);

        emit Withdraw(caller, receiver, owner, assets, shares);
    }

Note: This finding (being flawed at the code logic) is distinctly different from L-01 (merely touching on dodging through frontrunning) on Pashov's audit report.

Tools Used

Manual

Recommended Mitigation Steps

Refactor the check that will also have the owner checked for the role:

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

-    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

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #7

[c4-pre-sort]

raymondfam marked the issue as duplicate of #666

[c4-judge]

fatherGoose1 marked the issue as satisfactory