Sturdy contest
Findings & Analysis Report
2022-06-29
Table of contents
- Summary
- Scope
- Severity Criteria
-
- [M-01] Possible lost msg.value
- [M-02]
UNISWAP_FEEis hardcoded which will lead to significant losses compared to optimal routing - [M-03]
processYield()anddistributeYield()may run out of gas and revert due to long list of extra rewards/yields - [M-04] ConvexCurveLPVault’s
_transferYieldcan become stuck with zero reward transfer - [M-05] Withdrawing ETH collateral with max uint256 amount value reverts transaction
- [M-06] Yield can be unfairly divided because of MEV/Just-in-time stablecoin deposits
-
Low Risk and Non-Critical Issues
- Summary
- L-01 Mistaken null values cause
distributeYield()to revert - L-02 Can’t remove old assets
- L-03 Missing checks for
approve()’s return status - L-04 Move
ETHconstant to child contract - L-05 Unsafe casts and usage of
IERC20Detailed - L-06 Unused
receive()function will lock Ether in contract - L-07
safeApprove()is deprecated - L-08 Missing checks for
address(0x0)when assigning values toaddressstate variables - L-09 Open TODOs
- N-01
overridefunction arguments that are unused should have the variable name removed or commented out to avoid compiler warnings - N-02
publicfunctions not called by the contract should be declaredexternalinstead - N-03
constants should be defined rather than using magic numbers - N-04 Redundant cast
- N-05 Missing event for critical parameter change
- N-06 Use a more recent version of solidity
- N-07 Use a more recent version of solidity
- N-08 Variable names that consist of all capital letters should be reserved for
const/immutablevariables - N-09 NatSpec is incomplete
- N-10 Event is missing
indexedfields - N-11 Consider allowing the passing of a referral code
- N-12 Remove commented out code
- N-13 Consider two-phase ownership transfer
-
- Summary
- G-01 Add
require()for asset address checks before doing the exchange - G-02 Add an
unregisterAsset()function - G-03 Multiple
addressmappings can be combined into a singlemappingof anaddressto astruct, where appropriate - G-04 State variables should be cached in stack variables rather than re-reading them from storage
- G-05
internalfunctions only called once can be inlined to save gas - G-06
<array>.lengthshould not be looked up in every loop of afor-loop - G-07 Not using the named return variables when a function returns, wastes deployment gas
- G-08 Use a more recent version of solidity
- G-09 Using
> 0costs more gas than!= 0when used on auintin arequire()statement - G-10 It costs more gas to initialize variables to zero than to let the default of zero be applied
- G-11
internalfunctions not called by the contract should be removed to save deployment gas - G-12
++icosts less gas thani++, especially when it’s used infor-loops (--i/i--too) - G-13 Using
privaterather thanpublicfor constants, saves gas - G-14 Duplicated
require()/revert()checks should be refactored to a modifier or function - G-15 Empty blocks should be removed or emit something
- G-16 Functions guaranteed to revert when called by normal users can be marked
payable - G-17
publicfunctions not called by the contract should be declaredexternalinstead
- Disclosures
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit contest outlined in this document, C4 conducted an analysis of the Sturdy smart contract system written in Solidity. The audit contest took place between May 13—May 15 2022.
Wardens
73 Wardens contributed reports to the Sturdy contest:
- Picodes
- hyh
- berndartmueller
- WatchPug (jtp and ming)
- IllIllI
- sorrynotsorry
- jonah1005
- StErMi
- leastwood
- MaratCerby
- mtz
- Dravee
- rotcivegaf
- 0x52
- dipp
- cccz
- AuditsAreUS
- defsec
- 0x1f8b
- simon135
- robee
- 0xliumin
- 0xNazgul
- oyc_109
- joestakey
- 0xf15ers (remora and twojoy)
- hake
- 0x4non
- fatherOfBlocks
- GimelSec (rayn and sces60107)
- Funen
- p4st13r4 (0x69e8 and 0xb4bb4)
- 0xkatana
- Hawkeye (0xwags and 0xmint)
- Waze
- sikorico
- delfin454000
- mics
- bobirichman
- kebabsec (okkothejawa and FlameHorizon)
- TerrierLover
- BouSalman
- csanuragjain
- p_crypt0
- tintin
- cryptphi
- AlleyCat
- z3s
- hansfriese
- Tomio
- Cityscape
- SooYa
- ignacio
- JC
- Fitraldys
- 0v3rf10w
- samruna
- CertoraInc (egjlmn1, OriDabush, ItayG, and shakedwinder)
- isamjay
- peritoflores
- tabish
- pedroais
- saian
- sseefried
This contest was judged by hickuphh3. The judge also competed in the contest as a warden, but forfeited their winnings.
Final report assembled by liveactionllama.
Summary
The C4 analysis yielded an aggregated total of 8 unique vulnerabilities. Of these vulnerabilities, 2 received a risk rating in the category of HIGH severity and 6 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 43 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 40 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 Sturdy contest repository, and is composed of 5 smart contracts written in the Solidity programming language and includes 366 lines of Solidity code.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities according to a methodology based on OWASP standards.
Vulnerabilities are divided into three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
Further information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website.
High Risk Findings (2)
[H-01] Hard-coded slippage may freeze user funds during market turbulence
Submitted by jonah1005, also found by berndartmueller, Picodes, IllIllI, sorrynotsorry, and WatchPug
GeneralVault.sol#L125
GeneralVault set a hardcoded slippage control of 99%. However, the underlying yield tokens price may go down.
If Luna/UST things happen again, users’ funds may get locked.
LidoVault.sol#L130-L137
Moreover, the withdrawal of the lidoVault takes a swap from the curve pool. 1 stEth worth 0.98 ETH at the time of writing.
The vault can not withdraw at the current market.
Given that users’ funds would be locked in the lidoVault, I consider this a high-risk issue.
Proof of Concept
Recommended Mitigation Steps
There are different ways to set the slippage.
The first one is to let users determine the maximum slippage they’re willing to take. The protocol front-end should set the recommended value for them.
function withdrawCollateral(
address _asset,
uint256 _amount,
address _to,
uint256 _minReceiveAmount
) external virtual {
// ...
require(withdrawAmount >= _minReceiveAmount, Errors.VT_WITHDRAW_AMOUNT_MISMATCH);
}The second one is have a slippage control parameters that’s set by the operator.
// Exchange stETH -> ETH via Curve
uint256 receivedETHAmount = CurveswapAdapter.swapExactTokensForTokens(
_addressesProvider,
_addressesProvider.getAddress('STETH_ETH_POOL'),
LIDO,
ETH,
yieldStETH,
maxSlippage
); function setMaxSlippage(uint256 _slippage) external onlyOperator {
maxSlippage = _slippage;
//@audit This action usually emit an event.
emit SetMaxSlippage(msg.sender, slippage);
}These are two common ways to deal with this issue. I prefer the first one.
The market may corrupt really fast before the operator takes action.
It’s nothing fun watching the number go down while having no option.
sforman2000 (Sturdy) confirmed
Fix the issue of require 99% of withdrawAmount sturdyfi/code4rena-may-2022#11
I realise there are 2 issues discussed here:
- The high-risk severity relates to
GeneralVault’s tight 1% slippage. Because it is inherited by vaults, it can cause withdrawals to fail and for user funds to be stuck.- However, in the context of the LIDO vault specifically, #69’s first low-severity issue rightly points out that users can choose to withdraw their funds in stETH, then do conversion to ETH separately afterwards. Hence, funds won’t actually be stuck. I would’ve therefore classified this a medium-severity issue. Nevertheless, it is expected that users will attempt to withdraw to ETH instead of stETH in times of market volatility.
[H-02] The check for value transfer success is made after the return statement in _withdrawFromYieldPool of LidoVault
Submitted by pedroais, also found by 0x52, 0xliumin, cccz, CertoraInc, fatherOfBlocks, GimelSec, hake, hickuphh3, hyh, IllIllI, isamjay, mtz, oyc_109, p4st13r4, peritoflores, rotcivegaf, sorrynotsorry, StErMi, tabish, WatchPug, z3s, 0x4non, 0xf15ers, berndartmueller, dipp, Dravee, MaratCerby, saian, simon135, sseefried, and TerrierLover
Users can lose their funds
Proof of Concept
The code checks transaction success after returning the transfer value and finishing execution. If the call fails the transaction won’t revert since require(sent, Errors.VTCOLLATERALWITHDRAW_INVALID); won’t execute.
Users will have withdrawn without getting their funds back.
Recommended Mitigation Steps
Return the function after the success check
sforman2000 (Sturdy) confirmed
Fix the issue of return before require sturdyfi/code4rena-may-2022#9
Issue is rather clear-cut.
Medium Risk Findings (6)
[M-01] Possible lost msg.value
Submitted by rotcivegaf, also found by AuditsAreUS, berndartmueller, cccz, MaratCerby, dipp, and StErMi
GeneralVault.sol#L75-L89
LidoVault.sol#L79-L104
ConvexCurveLPVault.sol#L131-L149
Possible lost value in depositCollateral function call
Proof of Concept
In call depositCollateral can will send value and the asset can be an ERC20(!= address(0)), if LidoVault and ConvexCurveLPVault contract receive this call the fouds will lost.
Also in LidoVault, L88, if send as asset ETH(== address(0)) and send more value than _amount(msg.value > _amount), the exedent will lost.
Recommended Mitigation Steps
In GeneralVault, depositCollateral function:
- Check if the
msg.valueis zero when the_assetis ERC20(!= address(0)) - Check if the
msg.valueis equeal to_amountwhen the_assetETH(== address(0))
function depositCollateral(address _asset, uint256 _amount) external payable virtual {
if (_asset != address(0)) { // asset = ERC20
require(msg.value == 0, <AN ERROR FROM Errors LIBRARY>);
} else { // asset = ETH
require(msg.value == _amount, <AN ERROR FROM Errors LIBRARY>);
}
// Deposit asset to vault and receive stAsset
// Ex: if user deposit 100ETH, this will deposit 100ETH to Lido and receive 100stETH TODO No Lido
(address _stAsset, uint256 _stAssetAmount) = _depositToYieldPool(_asset, _amount);
// Deposit stAsset to lendingPool, then user will get aToken of stAsset
ILendingPool(_addressesProvider.getLendingPool()).deposit(
_stAsset,
_stAssetAmount,
msg.sender,
0
);
emit DepositCollateral(_asset, msg.sender, _amount);
}Also can remove the require(msg.value > 0, Errors.VT_COLLATERAL_DEPOSIT_REQUIRE_ETH); in LidoVault, L88
sforman2000 (Sturdy) confirmed
[M-02] UNISWAP_FEE is hardcoded which will lead to significant losses compared to optimal routing
Submitted by Picodes, also found by hickuphh3
In YieldManager, UNISWAP_FEE is hardcoded, which reduce significantly the possibilities and will lead to non optimal routes. In particular, all swaps using ETH path will use the wrong pool as it will use the ETH / USDC 1% one due to this line.
Proof of Concept
For example for CRV / USDC, the optimal route is currently CRV -> ETH and ETH -> USDC, and the pool ETH / USDC with 1% fees is tiny compared to the ones with 0.3 or 0.1%. Therefore using the current implementation would create a significant loss of revenue.
Recommended Mitigation Steps
Basic mitigation would be to hardcode in advance the best Uniswap paths in a mapping like it’s done for Curve pools, then pass this path already computed to the swapping library. This would allow for complex route and save gas costs as you would avoid computing them in swapExactTokensForTokens.
Then, speaking from experience, as distributeYield is onlyAdmin, you may want to add the possibility to do the swaps through an efficient aggregator like 1Inch or Paraswap, it will be way more optimal.
sforman2000 (Sturdy) confirmed
Fix the issue of hardcoded UNISWAP_FEE sturdyfi/code4rena-may-2022#12
[M-03] processYield() and distributeYield() may run out of gas and revert due to long list of extra rewards/yields
Submitted by IllIllI, also found by leastwood and StErMi
Yields will not be able to be distributed to lenders because attempts to do so will revert.
Proof of Concept
The processYield() function loops overall of the extra rewards and transfers them
File: smart-contracts/ConvexCurveLPVault.sol #1
105 uint256 extraRewardsLength = IConvexBaseRewardPool(baseRewardPool).extraRewardsLength();
106 for (uint256 i = 0; i < extraRewardsLength; i++) {
107 address _extraReward = IConvexBaseRewardPool(baseRewardPool).extraRewards(i);
108 address _rewardToken = IRewards(_extraReward).rewardToken();
109 _transferYield(_rewardToken);
110 }ConvexCurveLPVault.sol#L105-L110
There is no guarantee that the tokens involved will be efficient in their use of gas, and there are no upper bounds on the number of extra rewards:
function extraRewardsLength() external view returns (uint256) {
return extraRewards.length;
}
function addExtraReward(address _reward) external returns(bool){
require(msg.sender == rewardManager, "!authorized");
require(_reward != address(0),"!reward setting");
extraRewards.push(_reward);
return true;
}Even if not every extra reward token has a balance, an attacker can sprinkle each one with dust, forcing a transfer by this function
_getAssetYields() has a similar issue:
File: smart-contracts/YieldManager.sol #X
129 AssetYield[] memory assetYields = _getAssetYields(exchangedAmount);
130 for (uint256 i = 0; i < assetYields.length; i++) {
131 if (assetYields[i].amount > 0) {
132 uint256 _amount = _convertToStableCoin(assetYields[i].asset, assetYields[i].amount);
133 // 3. deposit Yield to pool for suppliers
134 _depositYield(assetYields[i].asset, _amount);
135 }
136 }Recommended Mitigation Steps
Include an offset and length as is done in YieldManager.distributeYield().
sforman2000 (Sturdy) confirmed
I’ve considered this issue. The reason why I chose not to raise it up is because adding reward tokens is restricted on Convex’s side. Given the number of integrations they have, it’s unlikely that they will add too many tokens or gas-guzzling ones that will cause claims to run OOG.
Nevertheless, it is a possibility, albeit an unlikely one, so I’ll let the issue stand (also since the sponsor confirmed it).
[M-04] ConvexCurveLPVault’s _transferYield can become stuck with zero reward transfer
Submitted by hyh
Now there are no checks for the amounts to be transferred via _transferYield and _processTreasury. As reward token list is external and an arbitrary token can end up there, in the case when such token doesn’t allow for zero amount transfers, the reward retrieval can become unavailable.
I.e. processYield() can be fully blocked for even an extended period, with some low probability, which cannot be controlled otherwise as pool reward token list is external.
Setting the severity to medium as reward gathering is a base functionality for the system and its availability is affected.
Proof of Concept
_transferYield proceeds with sending the amounts to treasury and yieldManager without checking:
ConvexCurveLPVault.sol#L74-L82
// transfer to treasury
if (_vaultFee > 0) {
uint256 treasuryAmount = _processTreasury(_asset, yieldAmount);
yieldAmount = yieldAmount.sub(treasuryAmount);
}
// transfer to yieldManager
address yieldManager = _addressesProvider.getAddress('YIELD_MANAGER');
TransferHelper.safeTransfer(_asset, yieldManager, yieldAmount);ConvexCurveLPVault.sol#L205-L209
function _processTreasury(address _asset, uint256 _yieldAmount) internal returns (uint256) {
uint256 treasuryAmount = _yieldAmount.percentMul(_vaultFee);
IERC20(_asset).safeTransfer(_treasuryAddress, treasuryAmount);
return treasuryAmount;
}The incentive token can be arbitrary. Some ERC20 do not allow zero amounts to be sent:
https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers
In a situation of such a token added to reward list and zero incentive amount earned the whole processYield call will revert, making reward gathering unavailable until either such token be removed from pool’s reward token list or some non-zero reward amount be earned. Both are external processes and aren’t controllable.
Recommended Mitigation Steps
Consider running the transfers in _transferYield only when yieldAmount is positive:
+ if (yieldAmount > 0) {
// transfer to treasury
if (_vaultFee > 0) {
uint256 treasuryAmount = _processTreasury(_asset, yieldAmount);
yieldAmount = yieldAmount.sub(treasuryAmount);
}
// transfer to yieldManager
address yieldManager = _addressesProvider.getAddress('YIELD_MANAGER');
TransferHelper.safeTransfer(_asset, yieldManager, yieldAmount);
+ }sforman2000 (Sturdy) confirmed
Fix the issue of some ERC20 tokens revert on zero value transfer sturdyfi/code4rena-may-2022#6
Agree with issue, there have been tokens that require the transfer amount to be non-zero. The other enabler is that the reward token list is arbitrary.
[M-05] Withdrawing ETH collateral with max uint256 amount value reverts transaction
Submitted by berndartmueller, also found by WatchPug
Withdrawing ETH collateral via the withdrawCollateral function using type(uint256).max for the _amount parameter reverts the transaction due to _asset being the zero-address and IERC20Detailed(_asset).decimals() not working for native ETH.
Proof of Concept
if (_amount == type(uint256).max) {
uint256 decimal = IERC20Detailed(_asset).decimals(); // @audit-info does not work for native ETH. Transaction reverts
_amount = _amountToWithdraw.mul(this.pricePerShare()).div(10**decimal);
}Recommended mitigation steps
Check _asset and use hard coded decimal value (18) for native ETH.
sforman2000 (Sturdy) confirmed
Fix the issue of transaction fails due to calculate ETH’s decimals sturdyfi/code4rena-may-2022#7
Good find! Stated in
_assetdescription that null address is interpreted as ETH, which isn’t a token, and therefore reverts when attempting to fetch its decimals.
[M-06] Yield can be unfairly divided because of MEV/Just-in-time stablecoin deposits
Submitted by mtz, also found by 0x52, hyh, jonah1005, leastwood, and sorrynotsorry
YieldManager.sol#L129-L134
YieldManager.sol#L160-L161
An attacker can use MEV (via gas auction or Flashbots or control of miners) to cause an unfair division of yield. By providing a very large (relative to the size of all other stablecoin deposits combined) stablecoin deposit Just-in-Time before an admin’s call to distributeYield the stablecoin deposited by the attacker will receive a very large amount of the yield and the attacker can immediately withdraw their deposit after yield is distributed. We assume this allows an attacker to get a lot of the yield reward even though they haven’t provided any deposit that has been borrowed. However, the exact mechanism for how yield is distributed to lenders of a particular stablecoin is in LendingPool.sol, which is out of scope. However it is implied in the documentation of this repo that it is based on the balance of that asset the lender has provided. We have confirmed that in LendingPool.sol the yield is distributed based on the proportion of the asset provided. However, even ignoring this, MEV can still be used to unfairly hurt lenders of other stablecoins.
Proof of Concept
- An attacker watches the mempool for calls to distributeYield by the admin.
- The attacker orders the block’s transactions (most easily using a flashbots bundle) in the following order:
i. Attacker deposits stablecoins to lend (ideally the stablecoin will be the one with the least volume).
ii. admin’s call to distributeYield happens.
iii. Attacker withdraws their deposit.
The attacker has thus made the asset they deposited (and thus themselves) receive much of the yield even though they provide no value to Sturdy since none of their deposit is ever borrowed so the never do anything to earn yield for sturdy. This attack can be done by a whale or by borrowing (even from sturdy) assets and converting them to a stablecoins accepted by sturdy before i. and returning them after iii. This will essentially be cost free for the attacker, none of their capital will ever be tied up by borrowers.
Recommended Mitigation Steps
The simplest way to mitigate this is for the admin to use flashbots or some other means of submitting the distributeYield call that skips the mempool. This is only a partial mitigation since attackers can still withdraw right after yield is distributed and get lucky by depositing soon before the distribution thus still capture more yield than they should have.
A better mitigation could use something like snapshotting who has deposited since the last yield distribution and only give these depositers yield based on the size of their deposits the next time yield is distributed.
sforman2000 (Sturdy) confirmed and commented:
We will use flashbots and vary when/how often yield is harvested to mitigate this.
hickuphh3 (judge) decreased severity to Medium and commented:
I take reference to discussions on Discord and in a thread below: https://github.com/code-423n4/2022-03-biconomy-findings/issues/135
To quote from 0xleastwood: “Protocol leaked value in has a broad context but I think most judges can agree that it would pertain to rewards being paid out a lower rate than expected. Or, users can extract small amounts (up to debate on what is considered to be small) from the protocol under certain assumptions.”
Hence, as per the TLDR risk assessment:
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.I would downgrade this to a medium severity.
Low Risk and Non-Critical Issues
For this contest, 43 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by IllIllI received the top score from the judge.
The following wardens also submitted reports: Dravee, sorrynotsorry, MaratCerby, 0x1f8b, defsec, 0xNazgul, robee, StErMi, BouSalman, mtz, p4st13r4, joestakey, oyc_109, berndartmueller, rotcivegaf, csanuragjain, 0xf15ers, hyh, dipp, hake, Hawkeye, WatchPug, delfin454000, sikorico, mics, 0xliumin, TerrierLover, 0x4non, p_crypt0, fatherOfBlocks, 0xkatana, GimelSec, bobirichman, tintin, Picodes, cryptphi, hickuphh3, Waze, Funen, AlleyCat, kebabsec, and simon135.
Summary
See original submission for details.
[L-01] Mistaken null values cause distributeYield() to revert
There are no null checks in the registerAsset() function, so admins can mistakenly pass 0x0 to that function, which will cause the for-loop to revert when that asset is reached. I’ve marked this as ‘Low’ rather than ‘Medium’ since the admin can work around it by using the _offset and _count input arguments to the function.
There is 1 instance of this issue:
File: smart-contracts/YieldManager.sol #1
118 function distributeYield(uint256 _offset, uint256 _count) external onlyAdmin {
119 // 1. convert from asset to exchange token via uniswap
120 for (uint256 i = 0; i < _count; i++) {
121 address asset = _assetsList[_offset + i];
122: require(asset != address(0), Errors.UL_INVALID_INDEX);[L-02] Can’t remove old assets
There is no way to remove old assets added by calls to registerAsset(). A disgruntled admin, before their access is revoked, can add a lot of assets and regularly sprinkle them with dust, so the new admins have to submit multiple calls to distributeYield() with different offsets and counts, to avoid the dust and possible reversion due to running out of gas
There is 1 instance of this issue:
File: smart-contracts/YieldManager.sol #1
118 function distributeYield(uint256 _offset, uint256 _count) external onlyAdmin {
119 // 1. convert from asset to exchange token via uniswap
120 for (uint256 i = 0; i < _count; i++) {
121 address asset = _assetsList[_offset + i];
122 require(asset != address(0), Errors.UL_INVALID_INDEX);
123 uint256 _amount = IERC20Detailed(asset).balanceOf(address(this));
124 _convertAssetToExchangeToken(asset, _amount);
125: }[L-03] Missing checks for approve()’s return status
Some tokens, such as Tether (USDT) return false rather than reverting if the approval fails. Use OpenZeppelin’s safeApprove(), which reverts if there’s a failure, instead
There is 1 instance of this issue:
File: smart-contracts/YieldManager.sol #1
221: IERC20(_asset).approve(_lendingPool, _amount);[L-04] Move ETH constant to child contract
All of the functions in the GeneralVault require 0x0 when referring to Ether, not the constant here. Having it here will lead to mistakes down the line. It’s only used by CurveswapAdapter, so it only needs to be there (it currently is also defined there).
There is 1 instance of this issue:
File: smart-contracts/GeneralVault.sol #1
47: address constant ETH = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;[L-05] Unsafe casts and usage of IERC20Detailed
The GeneralVault is meant to be general, i.e. not specific to Lido or Curve, and therefore should not assume that the asset will always be IERC20Detailed (not all ERC20 contracts define decimals() since it’s optional in the spec). Use safeDecimals() instead
There is 1 instance of this issue:
File: smart-contracts/GeneralVault.sol #1
122: uint256 decimal = IERC20Detailed(_asset).decimals();[L-06] Unused receive() function will lock Ether in contract
If the intention is for the Ether to be used, the function should call another function, otherwise it should revert
There is 1 instance of this issue:
File: smart-contracts/LidoVault.sol #1
24: receive() external payable {}[L-07] safeApprove() is deprecated
Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead
There are 3 instances of this issue:
File: smart-contracts/ConvexCurveLPVault.sol #1
141: IERC20(curveLPToken).safeApprove(convexBooster, _amount);File: smart-contracts/ConvexCurveLPVault.sol #2
146: IERC20(internalAssetToken).safeApprove(address(_addressesProvider.getLendingPool()), _amount);File: smart-contracts/LidoVault.sol #3
102: IERC20(LIDO).safeApprove(address(_addressesProvider.getLendingPool()), assetAmount);[L-08] Missing checks for address(0x0) when assigning values to address state variables
There is 1 instance of this issue:
File: smart-contracts/ConvexCurveLPVault.sol #1
41: curveLPToken = _lpToken;[L-09] Open TODOs
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment
There is 1 instance of this issue:
File: smart-contracts/GeneralVault.sol #1
77: // Ex: if user deposit 100ETH, this will deposit 100ETH to Lido and receive 100stETH TODO No Lido[N-01] override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings
There are 2 instances of this issue:
File: smart-contracts/ConvexCurveLPVault.sol #1
154: function _getWithdrawalAmount(address _asset, uint256 _amount)File: smart-contracts/LidoVault.sol #2
109: function _getWithdrawalAmount(address _asset, uint256 _amount)[N-02] public functions not called by the contract should be declared external instead
Contracts are allowed to override their parents’ functions and change the visibility from external to public.
There are 3 instances of this issue:
File: smart-contracts/CollateralAdapter.sol #1
35: function initialize(ILendingPoolAddressesProvider _provider) public initializer {File: smart-contracts/GeneralVault.sol #2
61: function initialize(ILendingPoolAddressesProvider _provider) public initializer {File: smart-contracts/YieldManager.sol #3
60: function initialize(ILendingPoolAddressesProvider _provider) public initializer {[N-03] constants should be defined rather than using magic numbers
There are 6 instances of this issue:
File: smart-contracts/ConvexCurveLPVault.sol
/// @audit 0xF403C135812408BFbE8713b5A23a04b3D48AAE31
40: convexBooster = 0xF403C135812408BFbE8713b5A23a04b3D48AAE31;File: smart-contracts/GeneralVault.sol
/// @audit 99_00
125: require(withdrawAmount >= _amount.percentMul(99_00), Errors.VT_WITHDRAW_AMOUNT_MISMATCH);
/// @audit 30_00
167: require(_fee <= 30_00, Errors.VT_FEE_TOO_BIG);File: smart-contracts/LidoVault.sol
/// @audit 200
48: 200
/// @audit 1e18
73: return 1e18;
/// @audit 200
136: 200[N-04] Redundant cast
The type of the variable is the same as the type to which the variable is being cast
There is 1 instance of this issue:
File: smart-contracts/LidoVault.sol #1
140: (bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');[N-05] Missing event for critical parameter change
There are 2 instances of this issue:
File: smart-contracts/ConvexCurveLPVault.sol #1
37 function setConfiguration(address _lpToken, uint256 _poolId) external onlyAdmin {
38 require(internalAssetToken == address(0), Errors.VT_INVALID_CONFIGURATION);
39
40 convexBooster = 0xF403C135812408BFbE8713b5A23a04b3D48AAE31;
41 curveLPToken = _lpToken;
42 convexPoolId = _poolId;
43 SturdyInternalAsset _interalToken = new SturdyInternalAsset(
44 string(abi.encodePacked('Sturdy ', IERC20Detailed(_lpToken).symbol())),
45 string(abi.encodePacked('c', IERC20Detailed(_lpToken).symbol())),
46 IERC20Detailed(_lpToken).decimals()
47 );
48 internalAssetToken = address(_interalToken);
49: }File: smart-contracts/YieldManager.sol #2
64 function setExchangeToken(address _token) external onlyAdmin {
65 require(_token != address(0), Errors.VT_INVALID_CONFIGURATION);
66 _exchangeToken = _token;
67: }[N-06] Use a more recent version of solidity
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
There are 3 instances of this issue:
File: smart-contracts/GeneralVault.sol #1
2: pragma solidity 0.6.12;File: smart-contracts/LidoVault.sol #2
2: pragma solidity 0.6.12;File: smart-contracts/YieldManager.sol #3
2: pragma solidity 0.6.12;[N-07] Use a more recent version of solidity
Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)
Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)
There is 1 instance of this issue:
File: smart-contracts/ConvexCurveLPVault.sol #1
2: pragma solidity 0.6.12;[N-08] Variable names that consist of all capital letters should be reserved for const/immutable variables
If the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).
There is 1 instance of this issue:
File: smart-contracts/LidoVault.sol #1
127: address LIDO = _addressesProvider.getAddress('LIDO');[N-09] NatSpec is incomplete
There are 2 instances of this issue:
File: smart-contracts/GeneralVault.sol #1
/// @audit Missing: '@return'
134 * @param _amount The amount to be withdrawn
135 */
136 function withdrawOnLiquidation(address _asset, uint256 _amount)
137 external
138 virtual
139: returns (uint256)File: smart-contracts/YieldManager.sol #2
/// @audit Missing: '@return'
104 * @param _tokenOut The address of token being received
105 */
106: function getCurvePool(address _tokenIn, address _tokenOut) external view returns (address) {[N-10] Event is missing indexed fields
Each event should use three indexed fields if there are three or more fields
There are 4 instances of this issue:
File: smart-contracts/GeneralVault.sol #1
24: event ProcessYield(address indexed collateralAsset, uint256 yieldAmount);File: smart-contracts/GeneralVault.sol #2
25: event DepositCollateral(address indexed collateralAsset, address indexed from, uint256 amount);File: smart-contracts/GeneralVault.sol #3
26: event WithdrawCollateral(address indexed collateralAsset, address indexed to, uint256 amount);File: smart-contracts/GeneralVault.sol #4
27: event SetTreasuryInfo(address indexed treasuryAddress, uint256 fee);[N-11] Consider allowing the passing of a referral code
There is 1 instance of this issue:
File: smart-contracts/GeneralVault.sol #1
81 ILendingPool(_addressesProvider.getLendingPool()).deposit(
82 _stAsset,
83 _stAssetAmount,
84 msg.sender,
85 0
86: );[N-12] Remove commented out code
There is 1 instance of this issue:
File: smart-contracts/GeneralVault.sol #1
144 // /**
145 // * @dev Convert an `amount` of asset used as collateral to swappable asset on liquidation.
146 // * @param _amountIn The amount of collateral asset
147 // */
148: // function convertOnLiquidation(address _assetOut, uint256 _amountIn) external virtual {}[N-13] Consider two-phase ownership transfer
Consider adding a two-phase transfer, where the current owner nominates the next owner, and the next owner has to call accept*() to become the new owner. This prevents passing the ownership to an account that is unable to use it.
There is 1 instance of this issue:
File: smart-contracts/YieldManager.sol #1
26: contract YieldManager is VersionedInitializable, Ownable {sforman2000 (Sturdy) commented:
Particularly high quality.
L1: Fixed
L2: Fixed
L3: Fixed
L4: Invalid. ETH constant may be used in several child contract.
L5: Admin can monitor it.
L6: Invalid. LidoVault can be received ETH from CurveSwap
L7: No need to change
L8: Fixed
L9: Fixed
N1: Fixed
N2: Fixed
N3: Invalid
N4: Fixed
N5: Fixed
N6: Fixed
N7: Fixed
N8: Fixed
N9: No need to change
N10: Invalid
N11: Invalid
N12: Fixed
N13: No need to change
L5 should be addressed IMO. there is an inconsistency between addresses being used. The LIDO vault should be using the ETH constant instead of the null address for ETH, or vice versa. For someone who uses etherscan, he’ll see the
ETHconstant define and assumes that he should be using that to specify ETH, then wonder why his tx will potentially revert in Metamask.
Low issues: L1, L2, L3, L4, L5, L7, centralisation risk
NC issues: L8, L9, N1, N2, N3 (more of sponsor acknowledged), N4, N5, N6, N7 (reasoning is diff from N6), N8, N9, N12, N13
Invalid: L6, N10 (some fields are not worth the extra gas to index), N11 (no justification)
Gas Optimizations
For this contest, 40 reports were submitted by wardens detailing gas optimizations. The report highlighted below by IllIllI received the top score from the judge.
The following wardens also submitted reports: Dravee, WatchPug, MaratCerby, defsec, simon135, rotcivegaf, joestakey, 0xliumin, robee, hansfriese, StErMi, Funen, hickuphh3, Tomio, Waze, 0xf15ers, Cityscape, 0xNazgul, 0x4non, 0xkatana, oyc_109, hake, SooYa, sikorico, ignacio, z3s, Hawkeye, kebabsec, JC, Fitraldys, delfin454000, GimelSec, mtz, 0v3rf10w, bobirichman, mics, samruna, 0x1f8b, and fatherOfBlocks.
Summary
| Issue | Instances | |
|---|---|---|
| 1 | Add require() for asset address checks before doing the exchange |
1 |
| 2 | Add an unregisterAsset() function |
1 |
| 3 | Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate |
1 |
| 4 | State variables should be cached in stack variables rather than re-reading them from storage | 7 |
| 5 | internal functions only called once can be inlined to save gas |
6 |
| 6 | <array>.length should not be looked up in every loop of a for-loop |
1 |
| 7 | Not using the named return variables when a function returns, wastes deployment gas | 1 |
| 8 | Use a more recent version of solidity | 5 |
| 9 | Using > 0 costs more gas than != 0 when used on a uint in a require() statement |
1 |
| 10 | It costs more gas to initialize variables to zero than to let the default of zero be applied | 5 |
| 11 | internal functions not called by the contract should be removed to save deployment gas |
2 |
| 12 | ++i costs less gas than i++, especially when it’s used in for-loops (--i/i-- too) |
5 |
| 13 | Using private rather than public for constants, saves gas |
5 |
| 14 | Duplicated require()/revert() checks should be refactored to a modifier or function |
2 |
| 15 | Empty blocks should be removed or emit something | 6 |
| 16 | Functions guaranteed to revert when called by normal users can be marked payable |
9 |
| 17 | public functions not called by the contract should be declared external instead |
3 |
Total: 61 instances over 17 issues
[G-01] Add require() for asset address checks before doing the exchange
The code below should verify that the address is either 0x0 or the LIDO address, in order to prevent wasting gas by doing all of the operations between this point and the actual check done in _depositToYieldPool()
There is 1 instance of this issue:
File: smart-contracts/LidoVault.sol #1
109 function _getWithdrawalAmount(address _asset, uint256 _amount)
110 internal
111 view
112 override
113 returns (address, uint256)
114 {
115 // In this vault, return same amount of asset.
116 return (_addressesProvider.getAddress('LIDO'), _amount);
117: }[G-02] Add an unregisterAsset() function
By unregistering and setting a mapping field to 0, you’ll be getting a Gsreset gas refund (2900 gas). If most registerAsset() calls are paired with unregisterAsset() calls, transactions will be cheaper
There is 1 instance of this issue:
File: smart-contracts/YieldManager.sol #1
73 function registerAsset(address _asset) external onlyAdmin {
74 _assetsList[_assetsCount] = _asset;
75 _assetsCount = _assetsCount + 1;
76: }[G-03] Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate
Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot
There is 1 instance of this issue:
File: smart-contracts/CollateralAdapter.sol #1
27 mapping(address => address) internal _assetToVaults;
28 // External collateral asset -> internal collateral asset
29: mapping(address => address) internal _collateralAssets;[G-04] State variables should be cached in stack variables rather than re-reading them from storage
The instances below point to the second+ access of a state variable within a function. Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read.
Less obvious fixes/optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 7 instances of this issue:
File: smart-contracts/ConvexCurveLPVault.sol
/// @audit convexBooster
142: IConvexBooster(convexBooster).deposit(convexPoolId, _amount, true);
/// @audit curveLPToken
138: TransferHelper.safeTransferFrom(curveLPToken, msg.sender, address(this), _amount);
/// @audit curveLPToken
141: IERC20(curveLPToken).safeApprove(convexBooster, _amount);
/// @audit internalAssetToken
146: IERC20(internalAssetToken).safeApprove(address(_addressesProvider.getLendingPool()), _amount);
/// @audit internalAssetToken
148: return (internalAssetToken, _amount);File: smart-contracts/YieldManager.sol
/// @audit _exchangeToken
202: address _pool = _curvePools[_exchangeToken][_tokenOut];
/// @audit _exchangeToken
207: _exchangeToken,[G-05] internal functions only called once can be inlined to save gas
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 6 instances of this issue:
File: smart-contracts/ConvexCurveLPVault.sol
205: function _processTreasury(address _asset, uint256 _yieldAmount) internal returns (uint256) {File: smart-contracts/LidoVault.sol
154: function _processTreasury(uint256 _yieldAmount) internal returns (uint256) {File: smart-contracts/YieldManager.sol
142: function _getAssetYields(uint256 _totalYieldAmount) internal view returns (AssetYield[] memory) {
178: function _convertAssetToExchangeToken(address asset, uint256 amount) internal {
195 function _convertToStableCoin(address _tokenOut, uint256 _amount)
196 internal
197: returns (uint256 receivedAmount)
219: function _depositYield(address _asset, uint256 _amount) internal {[G-06] <array>.length should not be looked up in every loop of a for-loop
The overheads outlined below are PER LOOP, excluding the first loop
- storage arrays incur a Gwarmaccess (100 gas)
- memory arrays use
MLOAD(3 gas) - calldata arrays use
CALLDATALOAD(3 gas)
Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There is 1 instance of this issue:
File: smart-contracts/YieldManager.sol #1
130: for (uint256 i = 0; i < assetYields.length; i++) {[G-07] Not using the named return variables when a function returns, wastes deployment gas
There is 1 instance of this issue:
File: smart-contracts/YieldManager.sol #1
200: return _amount;[G-08] Use a more recent version of solidity
Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath
Use a solidity version of at least 0.8.2 to get compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
There are 5 instances of this issue:
File: smart-contracts/CollateralAdapter.sol
2: pragma solidity 0.6.12;File: smart-contracts/ConvexCurveLPVault.sol
2: pragma solidity 0.6.12;File: smart-contracts/GeneralVault.sol
2: pragma solidity 0.6.12;File: smart-contracts/LidoVault.sol
2: pragma solidity 0.6.12;File: smart-contracts/YieldManager.sol
2: pragma solidity 0.6.12;[G-09] Using > 0 costs more gas than != 0 when used on a uint in a require() statement
This change saves 6 gas per instance
There is 1 instance of this issue:
File: smart-contracts/GeneralVault.sol #1
179: require(yieldStAsset > 0, Errors.VT_PROCESS_YIELD_INVALID);[G-10] It costs more gas to initialize variables to zero than to let the default of zero be applied
There are 5 instances of this issue:
File: smart-contracts/ConvexCurveLPVault.sol
106: for (uint256 i = 0; i < extraRewardsLength; i++) {File: smart-contracts/GeneralVault.sol
218: for (uint256 i = 0; i < length; i++) {File: smart-contracts/YieldManager.sol
120: for (uint256 i = 0; i < _count; i++) {
130: for (uint256 i = 0; i < assetYields.length; i++) {
156: for (uint256 i = 0; i < length; i++) {[G-11] internal functions not called by the contract should be removed to save deployment gas
If the functions are required by an interface, the contract should inherit from that interface and use the override keyword
There are 2 instances of this issue:
File: smart-contracts/GeneralVault.sol #1
204: function _getAssetYields(uint256 _WETHAmount) internal view returns (AssetYield[] memory) {File: smart-contracts/GeneralVault.sol #2
235: function _depositYield(address _asset, uint256 _amount) internal {[G-12] ++i costs less gas than i++, especially when it’s used in for-loops (--i/i-- too)
Saves 6 gas PER LOOP
There are 5 instances of this issue:
File: smart-contracts/ConvexCurveLPVault.sol
106: for (uint256 i = 0; i < extraRewardsLength; i++) {File: smart-contracts/GeneralVault.sol
218: for (uint256 i = 0; i < length; i++) {File: smart-contracts/YieldManager.sol
120: for (uint256 i = 0; i < _count; i++) {
130: for (uint256 i = 0; i < assetYields.length; i++) {
156: for (uint256 i = 0; i < length; i++) {[G-13] Using private rather than public for constants, saves gas
If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table
There are 5 instances of this issue:
File: smart-contracts/CollateralAdapter.sol
22: uint256 public constant VAULT_REVISION = 0x1;File: smart-contracts/GeneralVault.sol
55: uint256 public constant VAULT_REVISION = 0x1;File: smart-contracts/YieldManager.sol
41: uint256 public constant VAULT_REVISION = 0x1;
48: uint256 public constant UNISWAP_FEE = 10000; // 1%
49: uint256 public constant SLIPPAGE = 500; // 5%[G-14] Duplicated require()/revert() checks should be refactored to a modifier or function
Saves deployment costs
There are 2 instances of this issue:
File: smart-contracts/ConvexCurveLPVault.sol #1
101: require(_token == _tokenFromConvex, Errors.VT_INVALID_CONFIGURATION);File: smart-contracts/YieldManager.sol #2
203: require(_pool != address(0), Errors.VT_INVALID_CONFIGURATION);[G-15] Empty blocks should be removed or emit something
The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})
There are 6 instances of this issue:
File: smart-contracts/GeneralVault.sol
153: function processYield() external virtual {}
158: function pricePerShare() external view virtual returns (uint256) {}
246: {}
255: ) internal virtual returns (uint256) {}
265: {}File: smart-contracts/LidoVault.sol
24: receive() external payable {}[G-16] Functions guaranteed to revert when called by normal users can be marked payable
If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
There are 9 instances of this issue:
File: smart-contracts/CollateralAdapter.sol
43 function addCollateralAsset(
44 address _externalAsset,
45 address _internalAsset,
46 address _acceptVault
47: ) external onlyAdmin {File: smart-contracts/ConvexCurveLPVault.sol
37: function setConfiguration(address _lpToken, uint256 _poolId) external onlyAdmin {
87: function processYield() external override onlyAdmin {File: smart-contracts/GeneralVault.sol
165: function setTreasuryInfo(address _treasury, uint256 _fee) external onlyAdmin {File: smart-contracts/LidoVault.sol
30: function processYield() external override onlyAdmin {File: smart-contracts/YieldManager.sol
64: function setExchangeToken(address _token) external onlyAdmin {
73: function registerAsset(address _asset) external onlyAdmin {
92 function setCurvePool(
93 address _tokenIn,
94 address _tokenOut,
95 address _pool
96: ) external onlyAdmin {
118: function distributeYield(uint256 _offset, uint256 _count) external onlyAdmin {[G-17] public functions not called by the contract should be declared external instead
Contracts are allowed to override their parents’ functions and change the visibility from external to public and can save gas by doing so.
There are 3 instances of this issue:
File: smart-contracts/CollateralAdapter.sol #1
35: function initialize(ILendingPoolAddressesProvider _provider) public initializer {File: smart-contracts/GeneralVault.sol #2
61: function initialize(ILendingPoolAddressesProvider _provider) public initializer {File: smart-contracts/YieldManager.sol #3
60: function initialize(ILendingPoolAddressesProvider _provider) public initializer {sforman2000 (Sturdy) commented:
Particularly high quality.
3. Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate
This is not correct.
I have a simple test on remix and confirmed there is no effect. In fact the case of using struct type spent more gas (+65 gas)
Let me know your example.
2. Add an unregisterAsset() function
Yeah we need unregisterAsset function, but not sure about the resetting 0 should be efficient to reduce gas. Normally non-zero to non-zero is cheaper than zero to non-zero.
I think we don’t need additional feature to reset 0.
9. Using > 0 costs more gas than != 0 when used on a uint in a require() statement
This is also not correct.
I checked on remix with your same example, but greater is less than not equal.
greater: 30 gas
equal: 30 gas
greaterThan: 33 gas
notequal: 33 gas
Disclosures
C4 is an open organization governed by participants in the community.
C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.