Platform: Code4rena
Start Date: 24/10/2023
Pot Size: $36,500 USDC
Total HM: 4
Participants: 147
Period: 6 days
Judge: 0xDjango
Id: 299
League: ETH
Rank: 44/147
Findings: 3
Award: $126.34
๐ Selected for report: 1
๐ Solo Findings: 0
๐ Selected for report: 0xmystery
Also found by: 0x11singh99, 0xAadi, 0xAlix2, 0xG0P1, 0xStalin, 0xWaitress, 0x_Scar, 0xhacksmithh, 0xhunter, 0xpiken, Al-Qa-qa, Arz, Avci, Bauchibred, BeliSesir, Breeje, Bughunter101, DarkTower, Eeyore, Fitro, HChang26, Imlazy0ne, J4X, JCK, Kaysoft, Kral01, Madalad, Mike_Bello90, Noro, PASCAL, PENGUN, Proxy, Rickard, Shubham, SovaSlava, Strausses, Team_Rocket, ThreeSigma, Topmark, Udsen, Walter, Yanchuan, Zach_166, ZanyBonzy, adam-idarrha, adeolu, almurhasan, arjun16, ast3ros, asui, ayden, btk, cartlex_, castle_chain, cccz, chainsnake, codynhat, critical-or-high, cryptonue, csanuragjain, deepkin, degensec, dirk_y, erebus, foxb868, ge6a, hunter_w3b, jasonxiale, kkkmmmsk, lanrebayode77, lsaudit, marchev, matrix_0wl, max10afternoon, nuthan2x, oakcobalt, oxchsyston, pavankv, peanuts, pep7siup, pipidu83, pontifex, ptsanev, qpzm, radev_sw, rokinot, rotcivegaf, rvierdiiev, sorrynotsorry, squeaky_cactus, supersizer0x, tnquanghuy0512, twcctop, twicek, young, zhaojie, ziyou-
4.5226 USDC - $4.52
Project Name | Ethena Labs |
Repository | https://github.com/code-423n4/2023-10-ethena |
Website | Link |
Link | |
Documentation | Link |
Methods | Manual Review |
Total nSLOC | 588 over 6 contracts |
USDe.sol
EthenaMinting.sol
and the contract it extends, SingleAdminAccessControl.sol
StakedUSDeV2.sol
, the contract it extends, StakedUSDe.sol
and the additional contract it creates USDeSilo.sol
Contract | SLOC | Purpose | Libraries used |
---|---|---|---|
USDe.sol | 24 | USDe token stablecoin contract that grants another address the ability to mint USDe | @openzeppelin/ERC20Burnable.sol @openzeppelin/ERC20Permit.sol @openzeppelin/Ownable2Step.sol |
EthenaMinting.sol | 295 | The contract where minting and redemption occurs. USDe.sol grants this contract the ability to mint USDe | @openzeppelin/ReentrancyGuard.sol |
StakedUSDe.sol | 130 | Extension of ERC4626. Users stake USDe to receive stUSDe which increases in value as Ethena deposits protocol yield here | @openzeppelin/ReentrancyGuard.sol @openzeppelin/ERC20Permit.sol @openzeppelin/ERC4626.sol |
StakedUSDeV2.sol | 76 | Extends StakedUSDe, adds a redemption cooldown. | |
USDeSilo.sol | 20 | Contract to temporarily hold USDe during redemption cooldown | |
SingleAdminAccessControl.sol | 43 | EthenaMinting uses SingleAdminAccessControl rather than the standard AccessControl |
ID | Title | Severity |
---|---|---|
L-01 | The protocol does not ensure that the EthenaMinting.sol contract is the minter in the USDe contract, which can lead to impossible minting of USDe tokens for an indefinite period of time | Low |
L-02 | The _computeDomainSeparator() function doesn't fully comply with the rules for EIP-712: DOMAIN_SEPARATOR | Low |
L-03 | EIP712 has not been accurately applied to the Route struct | Low |
L-04 | Malicious actor can evade the FULL_RESTRICTED_STAKER_ROLE role | Low |
L-05 | getUnvestedAmount() is not needed to be read in StackedUSDe.sol#transferInRewards() function for the second time | Low |
NC-01 | Don't grant DEFAULT_ADMIN_ROLE twice in the EthenaMinting contract | Non Critical |
NC-02 | Write modifier for if (msg.sender != minter) revert OnlyMinter(); check in the USDe#mint() function, so to be consistent with USDe#setMinter() function which performs the access control check in modifier | Non Critical |
NC-03 | Change the names of removeFromBlacklist() and addToBlacklist() functions | Non Critical |
NC-04 | Failure to validate functions return values can result in errors | Non Critical |
NC-05 | Add check in rescueTokens() if the to parameter is FULL_RESTRICTED_STAKER_ROLE and if revert | Non Critical |
NC-06 | The System Exclusively Accepts Nonces | Non Critical |
NC-07 | When adding an asset to supportedAssets , perform a check to ensure that the supported asset is not the NATIVE_TOKEN . If it is, then revert | Non Critical |
NC-08 | Don't assign role on msg.sender | Non Critical |
NC-09 | It is recommended to use deadline for meta-transactions | Non Critical |
NC-10 | Lack of Event Emitting on Important for the Protocol functions | Non Critical |
EthenaMinting.sol
contract is the minter
in the USDe
contract, which can lead to a impossible minting of USDe
tokens for an indefinite periodThe Ethena protocol consists of two contracts, "USDe" and "EthenaMinting." The "USDe" contract allows tokens to be minted only by the designated "minter" address, and this address can be set via the "setMinter" function. The "EthenaMinting" contract attempts to mint USDe tokens via the "mint" function of the "USDe" contract. However, the vulnerability is that the "EthenaMinting" contract has not been properly set as the minter in the "USDe" contract. In such a scenario, USDe tokens cannot be minted until the "EthenaMinting" contract is granted the minter role in the "USDe" contract.
function mint(Order calldata order, Route calldata route, Signature calldata signature) external override nonReentrant onlyRole(MINTER_ROLE) belowMaxMintPerBlock(order.usde_amount) { if (order.order_type != OrderType.MINT) revert InvalidOrder(); verifyOrder(order, signature); if (!verifyRoute(route, order.order_type)) revert InvalidRoute(); if (!_deduplicateOrder(order.benefactor, order.nonce)) revert Duplicate(); // Add to the minted amount in this block mintedPerBlock[block.number] += order.usde_amount; _transferCollateral( order.collateral_amount, order.collateral_asset, order.benefactor, route.addresses, route.ratios ); usde.mint(order.beneficiary, order.usde_amount); emit Mint( msg.sender, order.benefactor, order.beneficiary, order.collateral_asset, order.collateral_amount, order.usde_amount ); }
function mint(address to, uint256 amount) external { if (msg.sender != minter) revert OnlyMinter(); _mint(to, amount); }
In the contest page write:
USDe minter - can mint any amount of USDe tokens to any address. Expected to be the EthenaMinting contract
So it is essential to set the minter
role in USDe.sol
contract still in USDe constructor
to EthenaMinting
contract, otherwise after deploying the Ethena Protocol minting from EthenaMinting
contract will be impossible.
EthenaMinting
contract will be impossible.USDe.sol
contract set the minter
to be the EthenaContract.sol
contract.So, implement the constructor of USDe.sol
contract as follow:
constructor(address admin, address ethenaMinting) ERC20("USDe", "USDe") ERC20Permit("USDe") { if (admin == address(0)) revert ZeroAddressException(); _transferOwnership(admin); minter = ethenaMinting; }
_computeDomainSeparator()
function doesn't fully comply with the rules for EIP-712: DOMAIN_SEPARATOR
The _computeDomainSeparator()
function doesn't fully comply with the rules for EIP-712: DOMAIN_SEPARATOR
. As indicated in this article, in the EIP-712: DOMAIN_SEPARATOR
section, it is mentioned that uint256
should be used instead of uint
(same for int
).
However, in the EthenaMinting.sol
contract, the DOMAIN_SEPARATOR
is generated as follows:
function _computeDomainSeparator() internal view returns (bytes32) { return keccak256(abi.encode(EIP712_DOMAIN, EIP_712_NAME, EIP712_REVISION, block.chainid, address(this))); }
As we can see from solidity documentation, the global/special variable block.chainid
is of type uint
by default, not uint256
.
Consequently, the _computeDomainSeparator
function doesn't explicitly cast block.chainid
to uint256
. This deviation means that the EthenaMinting
contract doesn't fully adhere to the rules for EIP-712: DOMAIN_SEPARATOR
.
Implement the _computeDomainSeparator
as follows:
function _computeDomainSeparator() internal view returns (bytes32) { return keccak256(abi.encode(EIP712_DOMAIN, EIP_712_NAME, EIP712_REVISION, uint256(block.chainid), address(this))); }
Route
structAs defined by the EIP712 standard, array values should be encoded as the keccak256
hash of the concatenated encodeData
of their contents. The current implementation in EthenaMinting.sol#encodeRoute()
does not adhere to this rule for the Route
Struct array fields.
Only, because the encodeRoute()
function is not invoked in the protocol, can be safely removed.
FULL_RESTRICTED_STAKER_ROLE
roleThe protocol has introduced the FULL_RESTRICTED_STAKER_ROLE
to empower the StakedUSDe
owner with the authority to blacklist
certain addresses and potentially manipulate their balances. However, this mechanism exhibits a vulnerability that could be bypassed with front funning attack.
StakedUSDe
owner decides to blacklist the user's address and initiates a transaction by calling addToBlacklist()
for that specific address.getUnvestedAmount()
is not needed to be read in StackedUSDe.sol#transferInRewards()
function for the second timeThe StackedUSDe.sol#transferInRewards()
function allows the owner to transfer rewards
from the controller contract
into this contract. However the function can be executed only if getUnvestedAmount()
is greater than zero:
if (getUnvestedAmount() > 0) revert StillVesting();
Later in the function newVestingAmount
used "add specified amount
from owner and getUnvestedAmount()
". However getUnvestedAmount()
will always be zero.
/** * @notice Allows the owner to transfer rewards from the controller contract into this contract. * @param amount The amount of rewards to transfer. */ 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); }
Rewrite the StackedUSDe.sol#transferInRewards()
function as follow:
/** * @notice Allows the owner to transfer rewards from the controller contract into this contract. * @param amount The amount of rewards to transfer. */ 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); + emit RewardsReceived(amount, amount); }
EthenaMinting
contractOne of the arguments in the constructor of the EthenaMinting
contract is admin
. In the constructor, it initially grants the DEFAULT_ADMIN_ROLE
to msg.sender
. Then, the code checks if msg.sender
is different from the admin
parameter, and if it is, the DEFAULT_ADMIN_ROLE
is granted to the admin
address. This logic is not efficient, and it is also inefficient for one contract to have two DEFAULT_ADMIN_ROLE
addresses.
constructor( IUSDe _usde, address[] memory _assets, address[] memory _custodians, address _admin, uint256 _maxMintPerBlock, uint256 _maxRedeemPerBlock ) { if (address(_usde) == address(0)) revert InvalidUSDeAddress(); if (_assets.length == 0) revert NoAssetsProvided(); - if (_admin == address(0)) revert InvalidZeroAddress(); + if (_admin == address(0) && msg.sender != admin) revert CUSTOM_ERROR(); usde = _usde; _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); for (uint256 i = 0; i < _assets.length; i++) { addSupportedAsset(_assets[i]); } for (uint256 j = 0; j < _custodians.length; j++) { addCustodianAddress(_custodians[j]); } // Set the max mint/redeem limits per block _setMaxMintPerBlock(_maxMintPerBlock); _setMaxRedeemPerBlock(_maxRedeemPerBlock); if (msg.sender != _admin) { _grantRole(DEFAULT_ADMIN_ROLE, _admin); } _chainId = block.chainid; _domainSeparator = _computeDomainSeparator(); emit USDeSet(address(_usde)); }
if (msg.sender != minter) revert OnlyMinter();
check in the USDe#mint()
function, so to be consistent with USDe#setMinter()
function which performs the access control check in modifierfunction setMinter(address newMinter) external onlyOwner { emit MinterUpdated(newMinter, minter); minter = newMinter; } function mint(address to, uint256 amount) external { if (msg.sender != minter) revert OnlyMinter(); _mint(to, amount); }
removeFromBlacklist()
and addToBlacklist()
functionsThe functions is not properly named, because the roles that are granted/revoked
are SOFT_RESTRICTED_STAKER_ROLE
and FULL_RESTRICTED_STAKER_ROLE
.
Within the StakedUSDe
contract, there are functions named addToBlacklist()
and removeFromBlacklist()
that utilize the _grantRole
and _revokeRole
methods respectively. It is essential to validate the return values of these functions in both scenarios. This ensures that when a role is granted, it confirms the target's absence of the role previously, and when a role is revoked, it confirms that the target indeed held this role.
rescueTokens()
if the to
parameter is FULL_RESTRICTED_STAKER_ROLE
and if revertFULL_RESTRICTED_STAKER_ROLE - address with this role can't burn his stUSDe tokens to unstake his USDe tokens, neither to transfer stUSDe tokens. His balance can be manipulated by the admin of StakedUSDe.
Every transaction that contains some sort of token transferring, revert if participant is someone with FULL_RESTRICTED_STAKER_ROLE
, expect rescueTokens()
function that transfer tokens accidentally sent to the contract. As the result owner can mistakenly transfer tokens to address with FULL_RESTRICTED_STAKER_ROLE
thinking that transfer tokens accidentally sent to the contract.
Add check in rescueTokens()
if the to
parameter is FULL_RESTRICTED_STAKER_ROLE
and if revert
Description: Presently, the nonce system implemented in _deduplicateOrder()
restricts us to having distinct slots for a user. Each slot accommodates bits. Nevertheless, in the _orderBitmaps
mapping, both key and value types are uint256, which implies that distinct nonces could unintentionally intersect, even when they shouldn't (e.g., and).
While it is improbable for nonces to reach such large values, we advise the team to address this concern, document the anticipated behavior in this situation, and implement any necessary corrections.
supportedAssets
, perform a check to ensure that the supported asset is not the NATIVE_TOKEN
. If it is, then revertThis will improve the code readability, because of the checks related to native tokens (ethers) performed in _transferCollateral()
will be no needed.
function _transferCollateral( uint256 amount, address asset, address benefactor, address[] calldata addresses, uint256[] calldata ratios ) internal { // cannot mint using unsupported asset or native ETH even if it is supported for redemptions if (!_supportedAssets.contains(asset) || asset == NATIVE_TOKEN) revert UnsupportedAsset(); IERC20 token = IERC20(asset); uint256 totalTransferred = 0; for (uint256 i = 0; i < addresses.length; ++i) { uint256 amountToTransfer = (amount * ratios[i]) / 10_000; token.safeTransferFrom(benefactor, addresses[i], amountToTransfer); totalTransferred += amountToTransfer; } uint256 remainingBalance = amount - totalTransferred; if (remainingBalance > 0) { token.safeTransferFrom(benefactor, addresses[addresses.length - 1], remainingBalance); } }
msg.sender
This a common smart contract security concern. In the specific context of Ethereum smart contracts, roles are used for access control: they designate which addresses have permission to perform certain actions, such as minting or burning tokens. When don't assign roles on msg.sender
leads to a more secure system of role management to prevent the abuse of critical permissions in the smart contract environment. It's a call for stronger security measures and more thoughtful control distribution, especially for functions that can significantly impact the tokenomics or the participants' trust in the platform.
Overall - This is bad solution. Better use approach like in UniswapV3.
The protocol currently lacks event emission in several crucial functions, which can hinder transparency and make tracking important actions challenging. Here are the instances where events are missing:
USDe.sol#mint()
: The mint()
function in the USDe contract does not emit an event when new tokens are minted, making it difficult to track and verify minting activities.EthenaMinting#disableMintRedeem()
: While the internal functions called within disableMintRedeem()
, such as _setMaxMintPerBlock()
and _setMaxRedeemPerBlock()
, emit events for the new set values, the main goal of disableMintRedeem()
differs. Adding an event specifically for disableMintRedeem
would provide clarity and enhance visibility.StakedUSDe#_deposit()
: The _deposit()
function in the StakedUSDe contract lacks event emission, making it challenging to monitor deposit activities.StakedUSDe#rescueTokens()
: The rescueTokens()
function does not emit an event, potentially causing issues in tracking rescued tokens.removeFromBlacklist()
, addToBlacklist()
: These functions in the protocol lack event emission, making it difficult to track changes to the blacklist.withdraw()
, redeem()
, unstake()
: These functions do not emit events, which can hinder monitoring and auditing processes.Adding appropriate events to these functions would greatly improve the protocol's transparency and auditability.
#0 - radeveth
2023-11-01T15:28:11Z
Actually the Migration Step
for [NC-01] Don't grant DEFAULT_ADMIN_ROLE twice in the EthenaMinting contract
should be -> https://gist.github.com/radeveth/b76bf7592315c7301edccc210b8bb275
My bad.
#1 - raymondfam
2023-11-02T01:54:17Z
False positives: NC-01, L-01, L-02 Pashov: L-03, L04, N-04 Bot: NC-10
#2 - c4-pre-sort
2023-11-02T01:54:24Z
raymondfam marked the issue as sufficient quality report
#3 - c4-judge
2023-11-14T17:09:05Z
fatherGoose1 marked the issue as grade-b
#4 - radeveth
2023-11-16T09:33:58Z
Hi, @raymondfam and @fatherGoose1! Thank you both for the quick judging.
I strongly believe that L-01
and NC-01
are valid findings.
For L-01
, the protocol does not ensure that the EthenaMinting.sol
contract has the minter role
in the USDe
contract. If you can show me where in the Solidity code this is happening, please provide the details.
Regarding NC-01
, I believe that this should be classified as a non-critical severity vulnerability, as granting two DEFAULT_ADMIN_ROLE
roles increases the centralization of the protocol twice.
#5 - radeveth
2023-11-22T16:56:15Z
Hello, @fatherGoose1!
Due to the comment above I think that my QA Report should be classified as A grade*.
๐ Selected for report: 0xVolcano
Also found by: 0x11singh99, 0xAadi, 0xAnah, 0xgrbr, 0xhacksmithh, 0xhex, 0xpiken, 0xta, J4X, JCK, K42, Raihan, Rolezn, SAQ, SM3_SS, Sathish9098, SovaSlava, ThreeSigma, Udsen, arjun16, aslanbek, brakelessak, castle_chain, evmboi32, hunter_w3b, lsaudit, naman1778, niser93, nuthan2x, oakcobalt, pavankv, petrichor, phenom80, radev_sw, shamsulhaq123, tabriz, thekmj, unique, yashgoel72, ybansal2403
6.4563 USDC - $6.46
ID | Title |
---|---|
GAS-01 | Prefer abi.encodePacked() over abi.encode() |
GAS-02 | Use bytes.concat() Instead of abi.encodePacked |
GAS-03 | Emit Memory Values Instead of Storage Values |
GAS-04 | Use assembly for math equations |
GAS-05 | Use assembly to validate msg.sender |
GAS-06 | Use the inputs/results of assignments rather than re-reading state variables |
GAS-07 | x + y is more efficient than using += for state variables (likewise for -=) |
abi.encodePacked()
over abi.encode()
For more information, refer to: Solidity Encode Gas Comparison
Issue Description:
There is one instance of this issue in the EthenaMinting.sol
contract:
๐ File: protocols/USDe/contracts/EthenaMinting.sol 49: bytes32 private constant EIP712_DOMAIN_TYPEHASH = keccak256(abi.encodePacked(EIP712_DOMAIN));
bytes.concat()
Instead of abi.encodePacked
When concatenation is not used for hashing, bytes.concat
is the preferred method as it is more gas-efficient.
Issue Description:
There is one instance of this issue in the EthenaMinting.sol
contract:
๐ File: protocols/USDe/contracts/EthenaMinting.sol 49: bytes32 private constant EIP712_DOMAIN_TYPEHASH = keccak256(abi.encodePacked(EIP712_DOMAIN));
Recommended Mitigation Steps:
Consider using bytes.concat()
and upgrading to at least Solidity version 0.8.4 if required.
Emit memory values instead of reading storage values again. This optimization can result in significant gas savings.
Gas Saved per Instance: ~97 (Total: ~582)
Issue Description: There are six instances of this issue:
๐ File: protocols/USDe/contracts/EthenaMinting.sol 439: emit MaxMintPerBlockChanged(oldMaxMintPerBlock, maxMintPerBlock); 446: emit MaxRedeemPerBlockChanged(oldMaxRedeemPerBlock, maxRedeemPerBlock);
๐ File: protocols/USDe/contracts/SingleAdminAccessControl.sol 28: emit AdminTransferRequested(_currentDefaultAdmin, newAdmin); 74: emit AdminTransferred(_currentDefaultAdmin, account);
๐ File: protocols/USDe/contracts/StakedUSDeV2.sol 133: emit CooldownDurationUpdated(previousDuration, cooldownDuration);
๐ File: protocols/USDe/contracts/USDe.sol 24: emit MinterUpdated(newMinter, minter);
Emitting memory values instead of reading storage values again can result in gas savings.
Replace (a * b) / c
with the assembly equivalent div(mul(a, b), c)
to save gas.
// Replace (a * b) / c with assembly uint256 result; assembly { result := div(mul(a, b), c) }
msg.sender
You can use assembly to efficiently validate msg.sender
. Here's an example of how to do this:
// Use assembly to validate msg.sender assembly { if iszero(eq(msg.sender, _admin)) { revert(0, 0) } }
You should use the result of assignments rather than re-reading state variables. Here's an example:
// Use the result of the assignment instead of re-reading the state variable uint256 newMaxMintPerBlock = maxMintPerBlock; maxMintPerBlock = newMaxMintPerBlock + increment; emit MaxMintPerBlockChanged(oldMaxMintPerBlock, maxMintPerBlock);
You should use x = x + y
instead of x += y
for state variables to save gas. Here's an example:
// Use x = x + y instead of x += y mintedPerBlock[block.number] = mintedPerBlock[block.number] + order.usde_amount;
#0 - c4-pre-sort
2023-11-01T15:08:41Z
raymondfam marked the issue as sufficient quality report
#1 - c4-judge
2023-11-10T20:31:11Z
fatherGoose1 marked the issue as grade-b
๐ Selected for report: radev_sw
Also found by: 0xSmartContract, 0xweb3boy, Al-Qa-qa, Bauchibred, Bulletprime, D_Auditor, J4X, JCK, K42, Kral01, Sathish9098, ZanyBonzy, albahaca, catellatech, clara, digitizeworx, fouzantanveer, hunter_w3b, invitedtea, jauvany, oakcobalt, pavankv, peanuts, xiao
115.3553 USDC - $115.36
# | Topic |
---|---|
1 | Architecture Overview (Protocol Explanation, Codebase Explanation, Examples Scenarios of Intended Protocol Flow) |
2 | Codebase Quality Analysis |
3 | Centralization Risks |
4 | Systemic Risks |
5 | Attack Vectors Discussed During the Audit |
6 | Example of Report that turned out to be invalid after I wrote a really good Explanation and PoC (The report explain the unstaking really well, so you can learn for it) |
Pdf link -> https://www.docdroid.net/pwoydhp/analysis-report-pdf
Overview:
Ethena is developing a DeFi ecosystem with a primary goal of offering a permissionless stablecoin, USDe, that allows users to earn yield within the system. This process is a contrast to traditional stablecoins like USDC, where the central authority (e.g., Circle) benefits from the yield. In Ethena's ecosystem, users can stake their USDe to earn stUSDe, which appreciates over time as the protocol generates yield.
Smart Contract Infrastructure:
USDe.sol
: The contract for the USDe stablecoin, limited in functionality with controls for minting privileges.EthenaMinting.sol
: This contract mints and redeems USDe in a single, atomic, trustless transaction. Central to user interactions, handling minting and redemption of USDe. It employs EIP712 signatures for transactions, routing collateral through predefined safe channels, and includes security measures against potential compromises by limiting minting and providing emergency roles (GATEKEEPERS) to intervene in suspicious activities.StakedUSDeV2.sol
: Allows USDe holders to stake their tokens for stUSDe, earning yield from the protocol's profits. It incorporates mechanisms to prevent exploitation of yield payouts and has a cooldown period for unstaking. For legal compliance, it can restrict certain users (based on jurisdiction or law enforcement directives) from staking or freeze their assets, with the provision of fund recovery in extreme cases.Roles in Ethena Ecosystem:
USDe
minter - can mint any amount of USDe tokens to any address. Expected to be the EthenaMinting contract.USDe
owner - can set token minter and transfer ownership to another addressUSDe
token holder - can not just transfer tokens but burn them and sign permits for others to spend their balanceStakedUSDe
admin - can rescue tokens from the contract and also to redistribute a fully restricted staker's stUSDe balance, as well as give roles to other addresses (for example the FULL_RESTRICTED_STAKER_ROLE role)StakedUSDeV2
admin - has all power of "StakedUSDe admin" and can also call the setCooldownDuration methodREWARDER_ROLE
- can transfer rewards into the StakedUSDe contract that will be vested over the next 8 hoursBLACKLIST_MANAGER_ROLE
- can do/undo full or soft restriction on a holder of stUSDeSOFT_RESTRICTED_STAKER_ROLE
- address with this role can't stake his USDe tokens or get stUSDe tokens minted to himFULL_RESTRICTED_STAKER_ROLE
- address with this role can't burn his stUSDe tokens to unstake his USDe tokens, neither to transfer stUSDe tokens. His balance can be manipulated by the admin of StakedUSDeMINTER_ROLE
- can actually mint USDe tokens and also transfer EthenaMinting's token or ETH balance to a custodian addressREDEEMER_ROLE
- can redeem collateral assets for burning USDeEthenaMinting admin
- can set the maxMint/maxRedeem amounts per block and add or remove supported collateral assets and custodian addresses, grant/revoke rolesGATEKEEPER_ROLE
- can disable minting/redeeming of USDe and remove MINTER_ROLE and REDEEMER_ROLE roles from authorized accountsWhat custodian is? (Chat GPT says)
custodianAddresses
likely refer to the collection of blockchain addresses that are authorized to hold assets on behalf of others.
These addresses are controlled by the custodian, which could be a smart contract or a third-party service that maintains the security of the private keys associated with these addresses.Users provide stETH as collateral to mint USDe. The system gives an RFQ (Request for Quote) detailing how much USDe they can create. Upon agreement, the user signs a transaction, and Ethena mints USDe against the stETH, which is then employed in various yield-generating activities, primarily shorting ETH perpetual futures to maintain a delta-neutral position.
Additional Explanations Related to the Minting
:
USDe
will be equivalent to using DAI, USDC, USDT (USDe
contract is just the ERC20 token for the stablecoin and this token will be the token used in StakedUSDeV2.sol
staking contract) where it doesn't have any yield
and only the holders of stUSDe
will earn the generated yield.EthenaMinting.sol
contract is the place where minting
is done.Ethena generates yield by taking advantage of the differences in staking returns (3-4% for stETH) and shorting ETH perpetuals (6-8%). Profits are funneled into an insurance fund and later distributed to stakers, enhancing the value of stUSDe relative to USDe.
Ethena employs a strategy involving stETH and short positions in ETH perpetuals, ensuring the value stability of users' holdings against market volatility.
Example โ 1:
Example โ 2:
USDe.sol
:
Code Organization:
Modifiers:
Ownable2Step
modifier, which enforces two-step ownership transfer.onlyRole
modifier is used to restrict certain functions to specific roles, enhancing security.Minting:
Gas Efficiency:
Overall, USDe.sol
appears to be well-structured and follows best practices for security and code organization.
EthenaMinting.sol
:
Code Organization:
Security Measures:
Signature Verification:
Gas Efficiency:
Domain Separator:
Deduplication:
Supported Assets:
Custodian Addresses:
Overall, EthenaMinting.sol
is well-structured, secure, and follows best practices for code organization and security.
StakedUSDe.sol:
The contract inherits Implementation of the ERC4626 "Tokenized Vault Standard" from OZ
 #### 
decimals
function is overridden to return the number of decimal places.StakedUSDeV2.sol:
StakedUSDe
and inherits its code organization structure.cooldowns
, silo
, MAX_COOLDOWN_DURATION
, and cooldownDuration
.ensureCooldownOff
and ensureCooldownOn
, are defined to control the execution of functions based on cooldown status.setCooldownDuration
function allows the admin to update the cooldown duration.USDeSilo.sol:
USDeSilo.sol
is to to hold the funds
for the cooldown period
whn user initiate unstaking
.General Observations:
SingleAdminAccessControl.sol:
In summary, the Ethena Protocol's codebase appears to be of high quality, with a strong focus on security and code organization.
Actually, the Ethena
Protocol contains many roles, each with quite a few abilities. This is necessary for the Protocol's logic and purpose.
The protocol assigns important roles like "MINTER," "REWARDER," and "ADMIN" to specific entities, potentially exposing the system to undue influence or risks if these roles are compromised.
So, these roles introduce several centralization risks. The most significant one is the scenario in which the MINTER
role becomes compromised. An attacker/minter could then mint a billion USDe
without collateral and dump them into pools, causing a black swan event that our insurance fund cannot cover.
However, Ethena
addresses this problem by enforcing on-chain mint and redeem limitations of 100k USDe per block."
From the documentation:
Our solution is to enforce an on chain mint and redeem limitation of 100k USDe per block. In addition, we have
GATEKEEPER
roles with the ability to disable mint/redeems and removeMINTERS
,REDEEMERS
.GATEKEEPERS
acts as a safety layer in case of compromisedMINTER
/REDEEMER
. They will be run in seperate AWS accounts not tied to our organisation, constantly checking each transaction on chain and disable mint/redeems on detecting transactions at prices not in line with the market. In case compromisedMINTERS
orREDEEMERS
after this security implementation, a hacker can at most mint 100k USDe for no collateral, and redeem all the collateral within the contract (we will hold ~$200k max), for a max loss of $300k in a single block, beforeGATEKEEPER
disable mint and redeem. The $300k loss will not materialy affect our operations.
In summary, Ethena
actually introduces several centralization risks due to the presence of many different roles in the Protocol. However, at the same time, the team has done its best to enforce measures that reduce the largest potential attack scenario to a maximum loss of $300k, which will not materially affect the Ethena
operations/ecosystem.
Hereโs an analysis of potential systemic
vulnerabilities
that can be exploited by attackers. If a smart contract has critical security flaws
, such as logic problems, this could lead to asset loss or system manipulation
. I strongly recommend that, once the protocol is audited, necessary actions be taken to mitigate any issues
identified by C4 Wardens
@openzeppelin/contracts-upgradeable
, and there is a risk that if any issues
are found with these dependencies
in your contracts, the Ethena
protocol could also be affected.I observed that old versions
of OpenZeppelin
are used in the project, and these should be updated to the latest version:
"name": "@openzeppelin/contracts-upgradeable", "description": "Secure Smart Contract library for Solidity", "version": "4.9.2",
The latest version is 4.9.3
(as of July 28, 2023), while the project uses version 4.9.2
.
EtehnaMinting.sol#mint()
(Minting Flow), EtehnaMinting.sol#redeem()
(Redemption Flow), StakedUSDe#deposit()/StakedUSDe#_deposit()
(Depositing Flow), StakedUSDeV2#unstake()
(Unstaking Flow).really good Explanation and PoC
(The report explain the unstaking
really well, so you can learn for it)withdraw/redeem
their assets/shares from StackedUSDeV2
contract. (Inefficient logic)StakedUSDeV2.sol
is where holders of USDe
stablecoin can stake their stablecoin, get stUSDe
in return and earn yield
. The Etehna Protocol yield is paid out by having a REWARDER
role of the staking contract send yield in USDe
, increasing the stUSDe
value with respect to USDe
. This contract is a modification of the ERC4626 standard
, with a change to vest in rewards linearly over 8 hours
When the unstake
process is initiated, from the user's perspective, stUSDe
is burnt immediately, and they will be able to invoke the withdraw()
function after cooldown
is up to get their USDe
in return. Behind the scenes, on burning of stUSDe
, USDe
is sent to a seperate USDeSilo contract to hold the funds for the cooldown period
. And on withdrawal
, the staking contract moves user funds from USDeSilo
contract out to the user's address
.
redemption
, starting of cooldown
and transferring of converted underlying asset to USSDeSilo contract
are cooldownAssets() and cooldownShares()function cooldownAssets(uint256 assets, address owner) external ensureCooldownOn returns (uint256) { if (assets > maxWithdraw(owner)) revert ExcessiveWithdrawAmount(); uint256 shares = previewWithdraw(assets); cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration; cooldowns[owner].underlyingAmount += assets; _withdraw(_msgSender(), address(silo), owner, assets, shares); return shares; } /// @notice redeem shares into assets and starts a cooldown to claim the converted underlying asset /// @param shares shares to redeem /// @param owner address to redeem and start cooldown, owner must allowed caller to perform this action function cooldownShares(uint256 shares, address owner) external ensureCooldownOn returns (uint256) { if (shares > maxRedeem(owner)) revert ExcessiveRedeemAmount(); uint256 assets = previewRedeem(shares); cooldowns[owner].cooldownEnd = uint104(block.timestamp) + cooldownDuration; cooldowns[owner].underlyingAmount += assets; _withdraw(_msgSender(), address(silo), owner, assets, shares); return assets; }
cooldown
is finished, user can call unstake()
to claim the staking amount after
.function unstake(address receiver) external { UserCooldown storage userCooldown = cooldowns[msg.sender]; uint256 assets = userCooldown.underlyingAmount; if (block.timestamp >= userCooldown.cooldownEnd) { userCooldown.cooldownEnd = 0; userCooldown.underlyingAmount = 0; silo.withdraw(receiver, assets); } else { revert InvalidCooldown(); } }
The function check if the cooldown is finished by block.timestamp >= userCooldown.cooldownEnd
check.
REMEMBER: The assets are transferred from USDeSilo
contract -> https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/USDeSilo.sol#L28-L30
Described logic above work only if the cooldown is set to number greater than zero. (aka the cooldown is active)
The Ethena
can decide to disable the cooldown period
, so the users to be able unstake without cooldown period
. If this is done, the user will be able to call directly withdraw()/redeem()
to unstake:
function withdraw(uint256 assets, address receiver, address owner) public virtual override ensureCooldownOff returns (uint256) { return super.withdraw(assets, receiver, owner); } /** * @dev See {IERC4626-redeem}. */ function redeem(uint256 shares, address receiver, address owner) public virtual override ensureCooldownOff returns (uint256) { return super.redeem(shares, receiver, owner); }
REMEMBER: The assets are transferred from StakedUSDeV2
contract -> https://github.com/code-423n4/2023-10-ethena/blob/main/contracts/USDeSilo.sol#L28-L30
So, the problem in Ethena protocol logic aries exactly when Ethena
decide to disable the cooldown period
.
Lets illustrate the following example:
USDe
tokens in StakedUSDeV2
contract.USDe
tokens in StakedUSDeV2
contract.cooldownAssets()
to start of cooldown period and converted underlying asset are transferred to USSDeSilo
contract and he redeem his 20000 USDe
tokens.Ethena
disable cooldown period.withdraw()/redeem()
.withdraw()/redeem()
he will not be able to get his converted underlying asset, because they are in USDeSilo
contract.withdraw()/redeem()
and get his converted underlying asset.After all, I observed that users who have already called cooldownAssets()/cooldownShares()
can call the unstake()
function again to retrieve their converted underlying asset.
35 hours
#0 - c4-pre-sort
2023-11-01T14:54:08Z
raymondfam marked the issue as high quality report
#1 - c4-sponsor
2023-11-09T19:58:31Z
FJ-Riveros (sponsor) acknowledged
#2 - c4-judge
2023-11-10T19:34:15Z
fatherGoose1 marked the issue as grade-a
#3 - c4-judge
2023-11-14T17:26:59Z
fatherGoose1 marked the issue as selected for report