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: 57/147
Findings: 2
Award: $95.19
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
Since the MAX_COOLDOWN_DURATION value is never changed within the contract, using a constant instead of a state variable is more gas-efficient and saves approximately 20,000 gas.
This is because constants are stored in memory, while state variables are stored in storage. Reading from and writing to storage is more expensive in terms of gas than reading from and writing to memory.
FILE: 2023-10-ethena/contracts/StakedUSDeV2.sol - 22: uint24 public MAX_COOLDOWN_DURATION = 90 days; + 22: uint24 public constant MAX_COOLDOWN_DURATION = 90 days;
It is generally safe to change uint256 to uint128 for variables that represent the maximum amount of something that can be minted or redeemed in a block on the Ethereum blockchain. The maximum amount of something that can be minted or redeemed in a block is unlikely to exceed 2^128 (340 undecillion).
Here are some specific examples of how this change has been implemented in existing protocols:
FILE: 2023-10-ethena/contracts/EthenaMinting.sol /// @notice max minted USDe allowed per block - 89: uint256 public maxMintPerBlock; + 89: uint128 public maxMintPerBlock; 90: ///Â @notice max redeemed USDe allowed per block - 91: uint256 public maxRedeemPerBlock; + 91: uint128 public maxRedeemPerBlock;
getUnvestedAmount()
function should be cachedit is a good practice to cache the getUnvestedAmount()
call in this case. This is because the getUnvestedAmount()
function may be expensive to compute, and caching the result can save gas.
FILE:Breadcrumbs2023-10-ethena/contracts/StakedUSDe.sol function transferInRewards(uint256 amount) external nonReentrant onlyRole(REWARDER_ROLE) notZero(amount) { + uint256 getUnvestedAmount_ = getUnvestedAmount() ; - if (getUnvestedAmount() > 0) revert StillVesting(); + if (getUnvestedAmount_ > 0) revert StillVesting(); - uint256 newVestingAmount = amount + getUnvestedAmount(); + 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); }
verifyOrder()
function for more gas efficiencySaves 2100 GAS
To improve gas efficiency, the taker_order_hash and signer can be cached after the function parameter checks. This avoids unnecessary external calls to ECDSA.recover() and the function call to hashOrder(). However, if any of the following checks revert:
Then the caching of taker_order_hash and signer is a waste of gas. Therefore, the code should be refactored to avoid caching in these cases. This saves gas
FILE: Breadcrumbs2023-10-ethena/contracts/EthenaMinting.sol /// @notice assert validity of signed order 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(); + 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(); return (true, taker_order_hash); }
Check parameters first then check external calls and state variables in If and Require checks when using || and && Operations
FILE: 2023-10-ethena/contracts/EthenaMinting.sol - 364: if (!_custodianAddresses.contains(route.addresses[i]) || route.addresses[i] == address(0) || route.ratios[i] == 0) + 364: if ( route.addresses[i] == address(0) || route.ratios[i] == 0 || !_custodianAddresses.contains(route.addresses[i]))
Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas) in a function that may ultimately revert in the unhappy case.
FILE: 2023-10-ethena/contracts/EthenaMinting.sol 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(); + if (!(signer == order.benefactor || delegatedSigner[signer][order.benefactor])) revert InvalidSignature(); return (true, taker_order_hash); }
FILE: 2023-10-ethena/contracts/EthenaMinting.sol IERC20 token = IERC20(asset); uint256 totalTransferred = 0; + uint256 amountToTransfer; for (uint256 i = 0; i < addresses.length; ++i) { - uint256 amountToTransfer = (amount * ratios[i]) / 10_000; + amountToTransfer = (amount * ratios[i]) / 10_000; token.safeTransferFrom(benefactor, addresses[i], amountToTransfer); totalTransferred += amountToTransfer; }
#0 - c4-pre-sort
2023-11-01T15:29:13Z
raymondfam marked the issue as sufficient quality report
#1 - c4-judge
2023-11-10T20:17: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
88.7348 USDC - $88.73
In the initial phase of our code review, conducted a thorough assessment of the project's scope. This assessment provided us with a clear understanding of the project's objectives, architecture, and the specific components that needed to be reviewed.
The USDe contract is responsible for the Ethena stablecoin. Only the minter address can mint USDe, and this address is always pointed to the EthenaMinting.sol contract.
The EthenaMinting contract is responsible for minting and redeeming USDe. It is only called by the Ethena backend
The StakedUSDeV2 contract allows users to stake their USDe and earn yield. There are some restrictions on who can use the staking contract and how they can use it, but these restrictions are in place to protect the protocol and its users.
EthenaMinting Contract
Validates and executes a mint order, transferring collateral from the benefactor to the Ethena protocol and minting USDe tokens for the beneficiary.
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 ); }
Validates and executes a redeem order, burning USDe tokens from the benefactor and transferring collateral to the beneficiary
function redeem(Order calldata order, Signature calldata signature) external override nonReentrant onlyRole(REDEEMER_ROLE) belowMaxRedeemPerBlock(order.usde_amount) { if (order.order_type != OrderType.REDEEM) revert InvalidOrder(); verifyOrder(order, signature); if (!_deduplicateOrder(order.benefactor, order.nonce)) revert Duplicate(); // Add to the redeemed amount in this block redeemedPerBlock[block.number] += order.usde_amount; usde.burnFrom(order.benefactor, order.usde_amount); _transferToBeneficiary(order.beneficiary, order.collateral_asset, order.collateral_amount); emit Redeem( msg.sender, order.benefactor, order.beneficiary, order.collateral_asset, order.collateral_amount, order.usde_amount ); }
Gnosis Multisig is a multi-signature wallet that allows multiple users to sign and approve transactions before they are executed. This makes it a more secure and reliable way to manage funds and assets, especially for large amounts or high-value transactions
Gnosis Multisig proxy architecture consists of the following components:
Gnosis Safe
: The Gnosis Safe is the main contract that provides the functionality of the multisig wallet. It is responsible for storing the wallet's assets and executing transactions.Gnosis SafeProxy
: The Gnosis SafeProxy is a proxy contract that sits between the Gnosis Safe and the Ethereum blockchain. It is responsible for forwarding transactions to the Gnosis Safe for execution.Gnosis SafeProxyFactory
: The Gnosis SafeProxyFactory is a contract that is used to create new Gnosis SafeProxy contracts.Using a Gnosis multisig to hold ownership of the smart contracts is a good security practice. A multisig wallet requires multiple signatures to approve transactions, which makes it more difficult for hackers to steal funds or otherwise disrupt the protocol.
It is also good that Ethena is using cold wallets for the multisig keys. Cold wallets are not connected to the internet, which makes them much more difficult to hack.
Requiring 7/10 or more confirmations before transactions are approved is a good way to ensure that the multisig is used securely. This means that at least 7 of the 10 multisig key holders must approve a transaction before it can be executed. This helps to protect against the possibility of a single key holder being compromised.
Overall, using a Gnosis multisig with cold wallets and 7/10 confirmations is a good way to secure the ownership of Ethena's smart contracts.
Slow decision-making
Centralization
If a small number of key holders control a large majority of the multisig keys, this could lead to centralization of power. This could make it easier for a small group of people to control Ethena and its future direction.Lockup risk
Mitigation Suggestions
The Gnosis Safe multisig wallet is a good choice, but there are other options that offer additional security features, such as hardware-based signing and multi-factor authentication.
A TEE is a secure environment within a processor that can be used to isolate and protect sensitive data. Using a TEE could help to protect the Ethena multisig keys from compromise.
The Ethena smart contracts currently rely on a centralized oracle to provide market prices. Using a decentralized oracle would make the protocol more resistant to manipulation
A circuit breaker is a mechanism that can be used to temporarily disable minting and redeems if there is a sharp movement in the price of ETH or stETH. This would help to prevent the Ethena protocol from being liquidated
Partnering with other DeFi protocols could help to increase the adoption of Ethena and provide users with more options for interacting with the protocol
mintedPerBlock
, redeemedPerBlock
, maxMintPerBlock
, and maxRedeemPerBlock
. SafeMath can be used for extra safety, even with Solidity 0.8.0 and later, to make the code more explicit about its intentions regarding avoiding overflows and underflows.modifiers
for repetitive access control checks
to reduce code duplication
and improve readability
events
to provide greater transparency and context for external systems
and users
. This can help with tracking
and auditing
validation checks
cover all possible edge cases and scenarios, reducing the risk of unexpected behaviorcomplex functions
into smaller
, more modular functions
with well-defined purposes. This can enhance code readability
and maintainability
naming conventions
for variables and functions. Make sure all names are self-descriptive
Here's an analysis of potential systemic and centralization risks in the contract:
Ethena delegates the custody of its collateral to third-party custodians
in order to achieve scalability
and security
. This means that Ethena does not hold the collateral itself
, but instead relies on the custodians
to keep it safe and secure
If a custodian is hacked, the hackers could steal the collateral that Ethena has deposited with them
- This would leave Ethena with insufficient collateral to back its USDe tokens, and the stablecoin peg would collapse.If a custodian goes bankrupt, it may not be able to return the collateral to Ethena
- This would also leave Ethena with insufficient collateral to back its USDe tokens, and the stablecoin peg would collapse.If a custodian is subject to a regulatory crackdown, it may be forced to liquidate Ethena's collateral
- This would also leave Ethena with insufficient collateral to back its USDe tokens, and the stablecoin peg would collapse.Mitigation Suggestions
Counterparty risk is the risk that one party to a financial contract will not fulfill its obligations. In the case of Ethena, the counterparty risk is the risk that the perps exchange that Ethena uses to generate yield on its collateral will become insolvent or default on its obligations
If this happens, Ethena could lose its collateral and be unable to mint new USDe tokens or redeem existing USDe tokens. This could have a number of negative consequences for Ethena users, including:
Mitigation Suggestions
Oracle risk is the risk that the price data provided by oracles is inaccurate or manipulated. In the case of Ethena, the oracle risk is the risk that the oracles that Ethena relies on to provide price data for ETH and stETH are compromised or malfunction.
If this happens, Ethena could misprice USDe and StakedUSDeV2. This could lead to a number of negative consequences for Ethena users, including:
Mitigation Suggestions
Sharp movements in the price of ETH or stETH could lead to losses for Ethena users. This is because Ethena maintains a delta-neutral position between stETH and short ETH perps. This means that Ethena buys stETH and shorts ETH perps in order to hedge its exposure to the price of ETH.
If the price of ETH or stETH moves sharply, Ethena may not be able to adjust its positions quickly enough. This could lead to Ethena being liquidated on its short ETH perps position or to Ethena losing money on its stETH holdings.
Ethena relies on a number of smart contracts, including USDe.sol
, EthenaMinting.sol
, and StakedUSDeV2.sol
. If there are any vulnerabilities in these contracts, they could be exploited by attackers to steal funds or otherwise disrupt the Ethena ecosystem.
// OpenZeppelin Contracts (last updated v4.9.0) (token/ERC20/ERC20.sol)
There are several important roles in the system.
bytes32 private constant MINTER_ROLE = keccak256("MINTER_ROLE");
Only addresses that have been assigned the MINTER_ROLE can successfully call the mint
and transferToCustody
functions.
bytes32 private constant REDEEMER_ROLE = keccak256("REDEEMER_ROLE");
Only addresses that have been assigned the MINTER_ROLE can successfully call the redeem()
function.
bytes32 private constant GATEKEEPER_ROLE = keccak256("GATEKEEPER_ROLE");
Only addresses that have been assigned the MINTER_ROLE can successfully call the disableMintRedeem()
,removeMinterRole()
,removeRedeemerRole()
functions.
It is also good that Ethena is using cold wallets for the multisig keys
. Cold wallets are not connected to the internet, which makes them much more difficult to hack.
Requiring 7/10
or more confirmations before transactions are approved is a good way to ensure that the multisig
is used securely
. This means that at least 7 of the 10 multisig key holders must approve a transaction before it can be executed. This helps to protect against the possibility of a single key holder being compromised.
What is the overall line coverage percentage provided by your tests?: 70%
The audit scope of the contracts should be the aim of reaching 100% test coverage. However, the tests provided by the protocol are at a high level of comprehension, giving us the opportunity to easily conduct Proof of Concepts (PoCs) to test certain bugs present in the project.
function redeem(Order calldata order, Signature calldata signature) external override nonReentrant onlyRole(REDEEMER_ROLE) belowMaxRedeemPerBlock(order.usde_amount) function mint(Order calldata order, Route calldata route, Signature calldata signature) external override nonReentrant onlyRole(MINTER_ROLE) belowMaxMintPerBlock(order.usde_amount) function transferToCustody(address wallet, address asset, uint256 amount) external nonReentrant onlyRole(MINTER_ROLE) {
The impact of a reentrancy vulnerability can be severe. An attacker can drain funds from the vulnerable contract, manipulate its state, or even render it unusable. Even reentrancy guard added should be tested.
Attack vectors, especially re-entrancy, seem untested and trusted.
10 Hours
10 hours
#0 - c4-pre-sort
2023-11-01T14:54:36Z
raymondfam marked the issue as sufficient quality report
#1 - c4-judge
2023-11-10T19:39:49Z
fatherGoose1 marked the issue as grade-a