Ethena Labs - btk's results

Lines of code

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

Vulnerability details

Impact

The SOFT_RESTRICTED_STAKER_ROLE is meant for users in countries where Ethena protocol cannot provide yield. These users are soft-restricted, preventing them from depositing USDe to get stUSDe or withdrawing stUSDe for USDe.

However, there is a significant oversight in the code. It doesn't prevent these users from withdrawing stUSDe for USDe. Consequently, a user who initially deposited in the StakedUSDeV2 vault and was later blacklisted can still earn yield and withdraw it as if they were a normal user, contradicting Ethena's legal requirements:

Due to legal requirements, there's a SOFT_RESTRICTED_STAKER_ROLE and FULL_RESTRICTED_STAKER_ROLE. The former is for addresses based in countries we are not allowed to provide yield to, for example USA.

Proof of Concept

Here's a coded PoC that demonstrates the issue:

  function testSoftRestrictedEarnAndWithdraw() public {
    address USA_USER = makeAddr("USA_USER");
    vm.label(USA_USER, "USA_USER");

    vm.prank(owner);
    stakedUSDe.grantRole(BLACKLIST_MANAGER_ROLE, alice);
    assertTrue(stakedUSDe.hasRole(BLACKLIST_MANAGER_ROLE, alice));

    _mintApproveDeposit(USA_USER, 100 ether);

    console.log("User balance balance:", usdeToken.balanceOf(USA_USER));

    vm.prank(alice);
    stakedUSDe.addToBlacklist(USA_USER, false);
    assertTrue(stakedUSDe.hasRole(SOFT_RESTRICTED_STAKER_ROLE, USA_USER));

    _transferRewards(1000 ether, 1000 ether);

    vm.warp(block.timestamp + 8 hours);

    _redeem(USA_USER, stakedUSDe.balanceOf(USA_USER));

    console.log("User balance After:", usdeToken.balanceOf(USA_USER));
  }

Test setup:

• Paste in: Paste in: StakedUSDeV2CooldownDisabledTest
• Run: forge test --mc StakedUSDeV2CooldownDisabledTest --mt testSoftRestrictedEarnAndWithdraw -vvv

[!NOTE]
Users can stay in the system indefinitely and continue earning yield.

Tools Used

Manual review

Recommended Mitigation Steps

To fix this issue, consider implementing the following changes:

• Prior to soft-blacklisting a user, withdraw his funds to their address.
• Add a check to prevent users with the SOFT_RESTRICTED_STAKER_ROLE from withdrawing stUSDe for USDe.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The BLACKLIST_MANAGER_ROLE has the ability to add/remove full/soft restrict any holder of stUSDe using addToBlacklist() and removeFromBlacklist(), both functions are safeguarded by the notOwner() modifier to ensure that the blacklist target is not the owner().

However, there's an edge case where the BLACKLIST_MANAGER_ROLE can still restrict the owner. This can happen by blacklisting the pending admin in the process of an ownership transfer.

[!NOTE]
Dev team confirmed: If it blacklists the owner, while it's not a concern it does go against the design of the smart contract. it will be a valid finding.

Proof of Concept

Here's a coded PoC that demonstrates the issue:

    function testBlacklistAdmin() public {
        vm.prank(owner);
        stakedUSDe.grantRole(BLACKLIST_MANAGER_ROLE, alice);
        assertTrue(stakedUSDe.hasRole(BLACKLIST_MANAGER_ROLE, alice));

        address pendingAdmin = makeAddr("pendingAdmin");
        vm.label(pendingAdmin, "pendingAdmin");

        vm.prank(owner);
        stakedUSDe.transferAdmin(pendingAdmin);

        // BLACKLIST_MANAGER_ROLE will blacklist the owner before he accept the ownership
        // It may happen intentionally(frontrunning acceptAdmin() or backrunning transferAdmin()) or mistakenly

        uint256 snapshot = vm.snapshot();

        // Alice will soft restrict the owner
        vm.prank(alice);
        stakedUSDe.addToBlacklist(pendingAdmin, false);
        assertTrue(
            stakedUSDe.hasRole(SOFT_RESTRICTED_STAKER_ROLE, pendingAdmin)
        );

        vm.prank(pendingAdmin);
        stakedUSDe.acceptAdmin();

        // BLACKLIST_MANAGER_ROLE succesfully blacklisted the owner
        console.log(
            "Is owner soft restricted!",
            stakedUSDe.hasRole(SOFT_RESTRICTED_STAKER_ROLE, stakedUSDe.owner())
        );

        vm.revertTo(snapshot);

        // Alice will full restrict the owner
        vm.prank(alice);
        stakedUSDe.addToBlacklist(pendingAdmin, true);
        assertTrue(
            stakedUSDe.hasRole(FULL_RESTRICTED_STAKER_ROLE, pendingAdmin)
        );

        vm.prank(pendingAdmin);
        stakedUSDe.acceptAdmin();

        // BLACKLIST_MANAGER_ROLE succesfully blacklisted the owner
        console.log(
            "Is owner full restricted!",
            stakedUSDe.hasRole(FULL_RESTRICTED_STAKER_ROLE, stakedUSDe.owner())
        );
    }

Test setup:

• Paste in StakedUSDeBlacklistTest
• Run: forge test --mc StakedUSDeBlacklistTest --mt testBlacklistAdmin -vvv

Tools Used

Manual review

Recommended Mitigation Steps

To address this issue, add the following function to SingleAdminAccessControl:

  function pendingOwner() public view virtual returns (address) {
    return _pendingDefaultAdmin;
  }

Then, update the notOwner() modifier as follows:

  modifier notOwner(address target) {
    if (target == owner() || target == pendingOwner()) revert CantBlacklistOwner();
    _;
  }

Assessed type

Other