Ethena Labs - castle_chain's results

Lines of code

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

Vulnerability details

Impact

this vulnerability will allow any attacker to stole the move his stUSDe after he get full restricted by the blacklist manager , which will allow the attacker to withdraw his stolen usde and then he can restake them with another account , so the full_Restricted_Role does not lock the funds in case of emergency .

Proof of Concept

As per README the FULL_RESTRICTED_STAKER_ROLE will be used in case of stolen funds , so the FULL_RESTRICTED_STAKER_ROLE should not be able to move his funds and unstake them so the ethena can repossess his stolen tokens , and the ethena should be able to repossess the funds of the FULL_RESTRICTED_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 .

, but in the stakedUsde.sol contract , the attcker which will be the owner of the stolen funds is allowed to withdraw his stusde tokens and get the usde just in case that the caller and the receiver is not a FULL_RESTRICTED_STAKER_ROLE , so the attacker can use an unrestricted account to withdraw his stolen funds . as shown in the function _withdraw() , the code checks that the caller and the receiver is not a FULL_RESTRICTED_STAKER_ROLE and then call the _withdraw function in the inherited ERC4626 contract . https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/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();
  }

then in the _withdraw function in the contract ERC4626 the contract checks that the owner approved the caller to _withdraw his shares , and then burns the shares stUSDe from the owner ,and then sends the Assets usde to the receiver . https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/lib/openzeppelin-contracts/contracts/token/ERC20/extensions/ERC4626.sol#L230-L251

    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);
    }

and the internal _burn() function will not revert if the owner is FULL_RESTRICTED_STAKER_ROLE since the to address is address(0) in the hook __beforeTokenTransfer() as shown here :

    function _burn(address account, uint256 amount) internal virtual {
        require(account != address(0), "ERC20: burn from the zero address");


@>      _beforeTokenTransfer(account, address(0), amount);
}

the _beforeTokenTransfer() function will not revert since the _burn() function passes the address to as the address(0) :

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

so in the process of withdrawing the stUsde the function withdraw does not revert in case of a FULL_RESTRICTED_STAKER_ROLE attacker tries to withdraw his stolen tokens .

the FULL_RESTRICTED_STAKER_ROLE attacker here needs a non-Full_restricted_account to call the function withdraw() and receive the tokens , and the FULL_RESTRICTED_STAKER_ROLE attacker also needs to be able to approve the stUsde amount to this account .

so even if the ethena protocol tried to repossess the attackers stUsde and called the function redistributeLockedAmount()` , the attacker will frontrun this transaction by setting higher gas price , so the attacker will withdraw the tokens .

Tools Used

manual review

Recommended Mitigation Steps

in order to mitigate this vulnerability , the protocol should consider apply one of this recommendations :

1. prevent approving the stUsde tokens by a FULL_RESTRICTED_STAKER_ROLE owner

    function _approve(address owner, address spender, uint256 amount) internal virtual {
        require(owner != address(0), "ERC20: approve from the zero address");
        require(spender != address(0), "ERC20: approve to the zero address");
+     if (hasRole(FULL_RESTRICTED_STAKER_ROLE, owner)) {
+        revert OperationNotAllowed();    

        _allowances[owner][spender] = amount;
        emit Approval(owner, spender, amount);
    }

2. set the receiver address to a governance contract in case of the owner is FULL_RESTRICTED_STAKER_ROLE and to address is the address(0) , to allow burning the shares ,but prevent withdrawing the usde by the FULL_RESTRICTED_STAKER_ROLE .

Assessed type

Invalid Validation

[L-01] the returned value of the function verifyOrder() should be checked

https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/EthenaMinting.sol#L339-L348

  function verifyOrder(Order calldata order, Signature calldata signature) public view override returns (bool, bytes32) {
    bytes32 taker_order_hash = hashOrder(order);
    address signer = ECDSA.recover(taker_order_hash, signature.signature_bytes);
    if (!(signer == order.benefactor || delegatedSigner[signer][order.benefactor])) revert InvalidSignature();
    if (order.beneficiary == address(0)) revert InvalidAmount();
    if (order.collateral_amount == 0) revert InvalidAmount();
    if (order.usde_amount == 0) revert InvalidAmount();
    if (block.timestamp > order.expiry) revert SignatureExpired();
    return (true, taker_order_hash);
  }

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

    verifyOrder(order, signature);

[L-02] a check can be removed to enhance the simplicity of the code

https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/EthenaMinting.sol#L353 the function verifyRoute() only get called in the function mint() which reverts if the orderType is REDEEM so this check can be removed .

    if (orderType == OrderType.REDEEM) {
      return true;
    }

  function mint(Order calldata order, Route calldata route, Signature calldata signature)
    external
    override
    nonReentrant
    onlyRole(MINTER_ROLE)
    belowMaxMintPerBlock(order.usde_amount)
  {
    if (!verifyRoute(route, order.order_type)) revert InvalidRoute();

in the function transferInRewards() , adding the value of the function getUnvestedAmount() can be removed to enhance the code , since the value of this function will always be zero

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

    if (getUnvestedAmount() > 0) revert StillVesting();
    uint256 newVestingAmount = amount + getUnvestedAmount();

the modifier notOwner() should be removed from the function removeFromBlacklist() since adding the owner to the blacklist is not possible .

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

the function removeFromBlacklist() revoke the blacklist role from the target address , so due to preventing the owner from being blacklisted , the modifier that checks of that the target is not the owner can be removed .

  function removeFromBlacklist(address target, bool isFullBlacklisting)
    external
    onlyRole(BLACKLIST_MANAGER_ROLE)
    notOwner(target)
  {
    bytes32 role = isFullBlacklisting ? FULL_RESTRICTED_STAKER_ROLE : SOFT_RESTRICTED_STAKER_ROLE;
    _revokeRole(role, target);
  }

[G-01] unnecessary check in the verifyRoute() function can be removed to save gas

the function verifyRoute only get called by the function mint() which reverts in case of the orderType is Redeem , so the internal check if the orderType is redeem can be removed https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/EthenaMinting.sol#L353-L355

    if (orderType == OrderType.REDEEM) {
      return true;
    }

[G-02] perform the checks first will save gas in case of revert .

in the function verifyOrder() the require statements should get executed first https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/EthenaMinting.sol#L342-L347

  function verifyOrder(Order calldata order, Signature calldata signature) public view override returns (bool, bytes32) {
    bytes32 taker_order_hash = hashOrder(order);
    address signer = ECDSA.recover(taker_order_hash, signature.signature_bytes);
    if (!(signer == order.benefactor || delegatedSigner[signer][order.benefactor])) revert InvalidSignature();
    if (order.beneficiary == address(0)) revert InvalidAmount();
    if (order.collateral_amount == 0) revert InvalidAmount();
    if (order.usde_amount == 0) revert InvalidAmount();
    if (block.timestamp > order.expiry) revert SignatureExpired();
    return (true, taker_order_hash);
  }

[G-03] unnecessary function calls can be removed to save gas

in the function transferInRewards() the function checks and reverts in case that the getUnvestedAmount is greater than zero so the function will only pass if the value of the function getUnvestedAmount() is equal to zero , but when the function transferInRewards() calculates the newVestingAmount the function adds the value of getUnvestedAmount which will always be equal to zero so this call to the getUnvestedAmount when calculating the newVestingAmount can be removed to save gas . https://github.com/code-423n4/2023-10-ethena/blob/ee67d9b542642c9757a6b826c82d0cae60256509/contracts/StakedUSDe.sol#L89-L99

  function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) {
@>  if (getUnvestedAmount() > 0) revert StillVesting();
@>  uint256 newVestingAmount = amount + getUnvestedAmount();


    vestingAmount = newVestingAmount;
    lastDistributionTimestamp = block.timestamp;
    // transfer assets from rewarder to this contract
    IERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);


    emit RewardsReceived(amount, newVestingAmount);
  }

[G-04] unnecessary modifier can be removed in order to save gas

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

the function removeFromBlacklist() revoke the blacklist role from the target address , so due to preventing the owner from being blacklisted , the modifier notOwner that checks of that the target is not the owner can be removed .

  function removeFromBlacklist(address target, bool isFullBlacklisting)
    external
    onlyRole(BLACKLIST_MANAGER_ROLE)
    notOwner(target)
  {
    bytes32 role = isFullBlacklisting ? FULL_RESTRICTED_STAKER_ROLE : SOFT_RESTRICTED_STAKER_ROLE;
    _revokeRole(role, target);
  }