Ethena Labs - Eeyore's results

Lines of code

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

Vulnerability details

The project requirements clearly state that a fully restricted account should not be able stake or unstake in the StakedUSDe contract.

However, in cases where an account becomes restricted after staking, it can easily bypass the restriction by granting approval to another account to withdraw on its behalf.

Impact

The restriction has no effect, and a restricted account can withdraw its stake without any obstacles.

Proof of Concept

1. Alice stakes 100 USDe.
2. Alice account is fully restricted by the BLACKLIST_MANAGER_ROLE.
3. Alice approves Bob to withdraw all her stUSDe balance.
4. Bob proceeds to redeem/withdraw all of Alice allowed stUSDe from the staking contract.

PoC Tests

This test demonstrates how to use the redeem() function to redeem all stUSDe belonging to Alice when her account is fully restricted.

To run this test, add it to the test/foundry/staking/StakedUSDe.blacklist.t.sol file and execute forge test --match-test test_PoC_fullBlacklist_user_can_bypassed_withdraw_restriction.

function test_PoC_fullBlacklist_user_can_bypassed_withdraw_restriction() public {
    usdeToken.mint(alice, _amount);

    // Alice deposit
    vm.startPrank(alice);
    usdeToken.approve(address(stakedUSDe), _amount);
    uint256 receivedStUSDe = stakedUSDe.deposit(_amount, alice);
    vm.stopPrank();

    // Alice fully blacklisted
    vm.prank(owner);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, alice);

    // Alice approves Bob
    vm.prank(alice);
    stakedUSDe.approve(bob, receivedStUSDe);

    uint256 balBefore = usdeToken.balanceOf(bob);

    // Bob is able to withdraw for Alice - with no restriction
    vm.startPrank(bob);
    vm.expectEmit(true, true, true, true);
    emit Withdraw(bob, bob, alice, _amount, receivedStUSDe);
    stakedUSDe.redeem(receivedStUSDe, bob, alice);
    vm.stopPrank();

    uint256 balAfter = usdeToken.balanceOf(bob);

    assertApproxEqAbs(_amount, balAfter - balBefore, 1);
}

Tools Used

Manual review

Recommended Mitigation Steps

To address this issue, update the _withdraw() function in the StakedUSDe contract by adding a check to ensure that _owner is not restricted:

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)) {
+   if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver) || hasRole(FULL_RESTRICTED_STAKER_ROLE, _owner)) {
      revert OperationNotAllowed();
    }

    super._withdraw(caller, receiver, _owner, assets, shares);
    _checkMinShares();
  }

Assessed type

Access Control

Lines of code

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

Vulnerability details

Project requirements clearly state that a fully restricted account should not be able to transfer, stake, or unstake.

However, in cases where an account is restricted, such an account can easily bypass the restriction by staking with a non-restricted account as the receiver for minted stUSDe.

Impact

The restriction has no effect, and a restricted account can deposit their USDe and receive yield.

Proof of Concept

1. Alice account is fully restricted by the BLACKLIST_MANAGER_ROLE.
2. Alice deposits 100 USDe, setting Bob as the receiver for stUSDe.
3. Bob receives stUSDe from Alice stake.

PoC Tests

This test illustrates how to use deposit() to stake USDe belonging to Alice in a situation when Alice was fully restricted.

Add this test to the test/foundry/staking/StakedUSDe.blacklist.t.sol and run forge test --match-test test_PoC_fullBlacklist_user_can_bypassed_deposit_restriction.

function test_PoC_fullBlacklist_user_can_bypassed_deposit_restriction() public {
    usdeToken.mint(alice, _amount);

    // Alice full blacklisted
    vm.prank(owner);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, alice);

    uint256 sharesBefore = stakedUSDe.balanceOf(bob);

    // Alice is able to deposit with Bob as receiver - with no restriction
    vm.startPrank(alice);
    usdeToken.approve(address(stakedUSDe), _amount);
    vm.expectEmit(true, true, true, true);
    emit Deposit(alice, bob, _amount, _amount);
    stakedUSDe.deposit(_amount, bob);
    vm.stopPrank();

    uint256 sharesAfter = stakedUSDe.balanceOf(bob);
    assertApproxEqAbs(sharesAfter - sharesBefore, _amount, 1);
}

Tools used

Manual review

Recommended Mitigation Steps

Update the _deposit() function in the StakedUSDe contract with a check to ensure that the receiver is not fully restricted (caller full restriction is checked in the _beforeTokenTransfer() function, duplication is not needed):

    function _deposit(address caller, address receiver, uint256 assets, uint256 shares) // @audit-ok
    internal
    override
    nonReentrant
    notZero(assets)
    notZero(shares)
  {
-   if (hasRole(SOFT_RESTRICTED_STAKER_ROLE, caller) || hasRole(SOFT_RESTRICTED_STAKER_ROLE, receiver)) {
+   if (hasRole(SOFT_RESTRICTED_STAKER_ROLE, caller) || hasRole(SOFT_RESTRICTED_STAKER_ROLE, receiver) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver)) {
      revert OperationNotAllowed();
    }
    super._deposit(caller, receiver, assets, shares);
    _checkMinShares();
  }

Assessed type

Access Control

Lines of code

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

Vulnerability details

In the unstake() function, the mechanism to claim staked assets after a cooldown relies on comparing the current block's timestamp with the cooldownEnd timestamp associated with the user:

...
if (block.timestamp >= userCooldown.cooldownEnd) { 
...

However, there's no provision in place to account for situations where the global cooldownDuration is set to 0. Consequently, users cannot instantly claim all their cooldowning assets under these circumstances.

Impact

If the global cooldownDuration is set to 0, the staking contract should behave like a standard ERC4626 vault, and users should be able to retrieve their full staked amount instantly upon withdrawal. Given that this is not the current behavior and users with active cooldowns face penalties compared to others, it could erode trust in the protocol. Users expect to access their assets seamlessly.

Proof of Concept

1. Bob stakes a certain amount, initiating a cooldown period.
2. For some reason, the protocol sets the global cooldownDuration to 0.
3. Bob tries to call the unstake() function to access his staked assets.
4. The function reverts, blocking Bob from accessing his assets, despite the expectation that he should be able to claim them instantly since the cooldown is effectively nullified.

Tools Used

Manual review

Recommended Mitigation Steps

It's suggested to introduce a condition to check whether the global cooldownDuration is set to 0, thereby enabling users to claim their staked assets immediately. This can be achieved with the code:

...
if (block.timestamp >= userCooldown.cooldownEnd || cooldownDuration == 0) {
...

Incorporating this condition ensures that users can access their staked assets without any issues when the global cooldown duration is set to zero.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L106-L109 https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/StakedUSDe.sol#L120-L123

Vulnerability details

Both of these issues stem from the same set of functions (addToBlacklist() and removeFromBlacklist()), which handle blacklisting operations in the contract.

Misleading NatSpec comments for roles

In the provided code for the addToBlacklist() and removeFromBlacklist() functions, the NatSpec comments imply that the DEFAULT_ADMIN_ROLE can directly interact with these functions. However, in reality, the DEFAULT_ADMIN_ROLE has the ability to control blacklisting indirectly by manipulating roles using the grantRole() and revokeRole() functions. This misleading documentation also hides the potential risk mentioned in the second issue where the DEFAULT_ADMIN_ROLE might inadvertently blacklist itself.

Potential for DEFAULT_ADMIN_ROLE to blacklist itself

An oversight exists where the DEFAULT_ADMIN_ROLE might blacklist itself, via the grantRole() function. While the addToBlacklist() function does have the notOwner() protection modifier, if it is genuinely intended that the DEFAULT_ADMIN_ROLE should also be able to blacklist but simultaneously be protected from blacklisting itself, additional logic is required. This could be achieved by overriding the _grantRole() function to include checks ensuring the owner does not blacklist themselves.

Impact

• Misleading documentation can cause developers or external integrators to misuse or misinterpret the functions.
• If the DEFAULT_ADMIN_ROLE can blacklist itself, it could render the admin ineffective, potentially stopping essential functions of the contract.

Proof of Concept

1. A user with the DEFAULT_ADMIN_ROLE attempts to use the addToBlacklist() and removeFromBlacklist() functions, but the calls are reverted.
2. A user with the DEFAULT_ADMIN_ROLE employs the grantRole() function and sets themselves as fully restricted.

Tools Used

Manual review

Recommended Mitigation Steps

1. Accurately update the NatSpec comments to reflect the capabilities of the DEFAULT_ADMIN_ROLE.
2. Implement checks to prevent the DEFAULT_ADMIN_ROLE from blacklisting themselves.
3. Override the _grantRole() function and incorporate the necessary checks to make sure only the intended addresses can be assigned specific roles.

Assessed type

Invalid Validation