Ethena Labs - Arz's results

Lines of code

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

Vulnerability details

The FULL_RESTRCITED_STAKER_ROLE is used for sanction/stolen funds and these addresses cannot move their funds and withdraw their stUSDe. The _withdraw() function is then overriden with these checks:

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

225:  function _withdraw(address caller, address receiver, address _owner, uint256 assets, uint256 shares)
226:      internal
227:      override
228:      nonReentrant
229:      notZero(assets)
230:      notZero(shares)
231:  {
232:    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver)) {
233:      revert OperationNotAllowed();
234:    }

The problem is that these checks only check the caller and the receiver and not the actual owner and if you take a look at the ERC4626.sol contract _withdraw() function, you can see that the owner of the shares can approve someone to withdraw on their behalf:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/0457042d93d9dfd760dbaa06a4d2f1216fdbe297/contracts/token/ERC20/extensions/ERC4626.sol#L255-L264

255: function _withdraw(
256:        address caller,
257:        address receiver,
258:        address owner,
259:        uint256 assets,
260:        uint256 shares
261:    ) internal virtual {
262:        if (caller != owner) {
263:            _spendAllowance(owner, caller, shares);
264:        }

So the blacklisted address would easily call approve() to approve a different non-blacklisted address that will then call withdraw and spend allowance - burning the shares of the blacklisted address and receiving the USDe

Impact

Fullblacklisted addresses can easily withdraw their funds that are supposed to be freeezed and avoid having their stolen/sanction funds seized.

Proof of Concept

Paste this into StakedUSDe.blacklist.t.sol and as you will see the FULL_RESTRICTED_STAKER_ROLE can easily withdraw funds by approving someone to withdraw on their behalf

function test_fullBlacklistApproveWithdraw() public {
    _mintApproveDeposit(alice, _amount, false);
    console.log("BALANCE OF ALICE BEFORE:", stakedUSDe.balanceOf(alice));
    
    // Alice gets blacklisted
    vm.startPrank(owner);
    stakedUSDe.grantRole(FULL_RESTRICTED_STAKER_ROLE, alice);
    vm.stopPrank();

    //Alice approves to Bob so he can withdraw
    vm.prank(alice);
    stakedUSDe.approve(bob, _amount);

    //Bob then withdraws Alices funds that are supposed to be freezed
    vm.prank(bob);
    stakedUSDe.redeem(_amount, makeAddr("receiver"), alice);

    console.log("BALANCE OF ALICE AFTER:", stakedUSDe.balanceOf(alice));
    console.log("USDe BALANCE OF RECEIVER:", usdeToken.balanceOf(makeAddr("receiver")));
  }

Tools Used

Foundry

Recommended Mitigation Steps

There needs to be a check in the overriden _withdraw() function that the _owner is not blacklisted

if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver) || hasRole(FULL_RESTRICTED_STAKER_ROLE, _owner) ) {
   revert OperationNotAllowed();
}

Assessed type

ERC4626

Lines of code

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

Vulnerability details

As the readme mentions, SOFT_RESTRICTED_STAKER_ROLE shouldnt be able to deposit or withdraw their USDe/stUSDe:

https://github.com/code-423n4/2023-10-ethena/tree/main#stakedusdev2sol

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. Addresses under this category will be soft restricted. They cannot deposit USDe to get stUSDe or withdraw stUSDe for USDe. However they can participate in earning yield by buying and selling stUSDe on the open market

The problem is that the _withdraw() function only checks if the addresses have FULL_RESTRCITED_STAKER_ROLE but not the SOFT_RESTRICTED_STAKER_ROLE

Impact

SOFT_RESTRICTED_STAKER_ROLE will be able to withdraw stUSDe for USDe even if it shouldnt as the docs mention.

Proof of Concept

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

225:  function _withdraw(address caller, address receiver, address _owner, uint256 assets, uint256 shares)
226:      internal
227:      override
228:      nonReentrant
229:      notZero(assets)
230:      notZero(shares)
231:  {
232:    if (hasRole(FULL_RESTRICTED_STAKER_ROLE, caller) || hasRole(FULL_RESTRICTED_STAKER_ROLE, receiver)) {
233:      revert OperationNotAllowed();
234:    }

As you can see the checks only check if the caller and receiver have the FULL_RESTRICTED_STAKER_ROLE but not the SOFT_RESTRICTED_STAKER_ROLE

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check for SOFT_RESTRICTED_STAKER_ROLE too in the overriden _withdraw() function

if (hasRole(SOFT_RESTRICTED_STAKER_ROLE, caller) || hasRole(SOFT_RESTRICTED_STAKER_ROLE, receiver)) {
   revert OperationNotAllowed();
}

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

If the user decides to withdraw while some of his funds are already in the cooldown period, the userCooldown.cooldownEnd will only keep increasing because its not unique for each of the users withdrawal.

The user will then have to wait much longer for all of his funds to get withdrawn even though he already waited some time for the first withdrawal.

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

100: cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration;

The user can have funds already in the cooldown period so the cooldownEnd will be set but if he decides to withdraw more then the cooldownEnd will be increased again.

Impact

The user will have to wait much longer to unstake all of his funds even though he already waited and will receive the funds later than supposed to.

Proof of Concept

Add this to StakedUSDeV2.cooldownEnabled.t.sol, this test demonstrates that the cooldownEnd keeps increasing and is not unique for each withdrawal of the user.

function testCooldownEnd() public {
    vm.prank(owner);
    stakedUSDe.setCooldownDuration(15 days);

    _mintApproveDeposit(alice, 100e18);

    uint256 shares = stakedUSDe.balanceOf(alice);

    //Alice decides to withdraw half of her shares
    vm.prank(alice);
    stakedUSDe.cooldownShares(shares / 2, alice);

    (uint104 cooldownEnd, ) = stakedUSDe.cooldowns(alice);
    vm.warp(block.timestamp + 7 days);
    console.log("The cooldown ends in %s days", (cooldownEnd - block.timestamp) / 1 days);

    //Alice then later decides that she wants to withdraw the rest
    //But the previous funds arent claimed yet
    vm.prank(alice);
    stakedUSDe.cooldownShares(shares/ 2, alice);

    //Alice now has to wait 15 days to claim all her funds even tho she already waited 7 days
    (uint104 cooldownEnd2, ) = stakedUSDe.cooldowns(alice);
    console.log("The cooldown now ends in %s days", (cooldownEnd2 - block.timestamp) / 1 days);
  }

Tools Used

Foundry

Recommended Mitigation Steps

Consider making each of the withdrawal unique by having 2 mappings and then the cooldownEnd will be different for each of the users withdrawal.

mapping(address => uint256) public id;
mapping(address => mapping(uint256 id => UserCooldown)) public cooldowns;

Assessed type

Timing