| title | sponsor | slug | date | findings | contest |
|---|---|---|---|---|---|
Backd contest |
Backd |
2022-04-backd |
2022-06-02 |
112 |
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 Backd smart contract system written in Solidity. The audit contest took place between April 21—April 27 2022.
62 Wardens contributed reports to the Backd contest:
- unforgiven
- IllIllI
- WatchPug
- 0xDjango
- shenwilly
- sseefried
- 0x52
- Ruhum
- fatherOfBlocks
- wuwe1
- defsec
- Dravee
- horsefacts
- joestakey
- pauliax
- StyxRave
- rayn
- hubble (ksk2345 and shri4net)
- robee
- antonttc
- Tomio
- csanuragjain
- TrungOre
- sorrynotsorry
- catchup
- 0xkatana
- reassor
- berndartmueller
- kenta
- securerodd
- gs8nrv
- hake
- z3s
- 0v3rf10w
- Funen
- TerrierLover
- oyc_109
- simon135
- Tadashi
- dipp
- kebabsec (okkothejawa and FlameHorizon)
- peritoflores
- jayjonah8
- Kenshin
- m4rio_eth
- remora
- MaratCerby
- 0x1f8b
- cccz
- hyh
- slywaters
- 0x4non
- 0xNazgul
- NoamYakov
- rfa
- saian
- 0xmint
- tin537
- danb
- UnusualTurtle
This contest was judged by gzeon. The judge also competed in the contest as a warden, but forfeited their winnings.
Final report assembled by liveactionllama.
The C4 analysis yielded an aggregated total of 18 unique vulnerabilities. Of these vulnerabilities, 3 received a risk rating in the category of HIGH severity and 15 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 39 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 35 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
The code under review can be found within the C4 Backd contest repository, and is composed of 38 smart contracts written in the Solidity programming language and includes 4,630 lines of Solidity code.
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.
Submitted by 0xDjango, also found by unforgiven
I believe this to be a high severity vulnerability that is potentially included in the currently deployed StakerVault.sol contract also. The team will be contacted immediately following the submission of this report.
In StakerVault.sol, the user checkpoints occur AFTER the balances are updated in the transfer() function. The user checkpoints update the amount of rewards claimable by the user. Since their rewards will be updated after transfer, a user can send funds between their own accounts and repeatedly claim maximum rewards since the pool's inception.
In every actionable function except transfer() of StakerVault.sol, a call to ILpGauge(lpGauge).userCheckpoint() is correctly made BEFORE the action effects.
Assume a certain period of time has passed since the pool's inception. For easy accounting, assume poolStakedIntegral of LpGauge.sol equals 1. The poolStakedIntegral is used to keep track of the current reward rate.
Steps:
- Account A stakes 1000 LP tokens.
balances[A] += 1000 - In the same
stakeFor()function,userCheckpoint()was already called so A will already haveperUserShare[A]set correctly based on their previously 0 balance and the currentpoolStakedIntegral. - Account A can immediately send all balance to Account B via
transfer(). - Since the checkpoint occurs after the transfer, B's balance will increase and then
perUserShare[B]will be updated. The calculation forperUserSharelooks as follows.
perUserShare[user] += (
(stakerVault.stakedAndActionLockedBalanceOf(user)).scaledMul(
(poolStakedIntegral_ - perUserStakedIntegral[user])
)
);Assuming Account B is new to the protocol, their perUserStakedIntegral[user] will default to 0.
perUserShare[B] += 1000 * (1 - 0) = 1000
- B is able to call
claimRewards()and mint all 1000 reward tokens. - B then calls
transfer()and sends all 1000 staked tokens to Account C. - Same calculation occurs, and C can claim all 1000 reward tokens.
- This process can be repeated until the contract is drained of reward tokens.
In StakerVault.transfer(), move the call to ILpGauge(lpGauge).userCheckpoint() to before the balances are updated.
chase-manning (Backd) confirmed and resolved
[H-02] function lockFunds in TopUpActionLibrary can cause serious fund lose. fee and Capped bypass. It's not calling stakerVault.increaseActionLockedBalance when transfers stakes.
Submitted by unforgiven
In function TopUpActionLibrary.lockFunds when transfers stakes from payer it doesn't call stakerVault.increaseActionLockedBalance for that payer so stakerVault.actionLockedBalances[payer] is not get updated for payer and stakerVault.stakedAndActionLockedBalanceOf(payer) is going to show wrong value and any calculation based on this function is gonna be wrong which will cause fund lose and theft and some restriction bypasses.
When user wants to create a TopUpAction. so he deposit his funds to Pool and get LP token. then stake the LP token in StakerVault and use that stakes to create a TopUp position with function TopUpAction.register. This function transfer user stakes (locks user staks) and create his position.
For transferring and locking user stakes it uses TopUpActionLibrary.lockFunds. function lockFunds transfers user stakes but don't call stakerVault.increaseActionLockedBalance for the payer which cause that stakerVault.actionLockedBalances[payer] to get different values(not equal to position.depositTokenBalance).
Function StakerVault.stakedAndActionLockedBalanceOf(account) uses stakerVault.actionLockedBalances[account] so it will return wrong value and any where in code that uses stakedAndActionLockedBalanceOf() is going to cause problems.
three part of the codes uses stakerVault.stakedAndActionLockedBalanceOf():
- LiqudityPool.depositFor() for checking user total deposits to be less than depositCap.
- LiqudityPool._updateUserFeesOnDeposit() for updating user fee on new deposits.
- userCheckpoint() for calculating user rewards.
attacker can use #1 and #2 to bypass high fee payment and max depositCap and #3 will cause users to lose rewards.
The detail steps:
1- user deposit fund to Pool and get LP token.
2- user stakes LP token in StakerVault.
3- user approve TopUpAction address to transfer his staks in StakerVault.
3- user use all his stakes to create a position with TopUpAction.register() function.
3.1- register() will call lockFunds to transfer and lock user stakes.
3.2- lockFunds() will transfer user stakes with stakerVault.transferFrom() but don't call stakerVault.increaseActionLockedBalance() so StakerVault.actionLockedBalances[user] will be zero.
3.3- StakerVault.balance[useer] will be zero too because his stakes get transfers in 3.2
4- StakerVault.stakedAndActionLockedBalanceOf(user) will return zero (user has some locked stakes in TopUpAction but because of the bug calculation get out of sync)
In this moment user will lose all the rewards that are minted in LpGauge. because userCheckpoint() use stakerVault.stakedAndActionLockedBalanceOf(user) for calculating rewards which is zero and new rewards will be zero too.
Attacker can use this process to bypass "max deposit Cap" and deposit any amount of assets he wants. because LiqudityPool.depositFor(address,uint256,uint256) uses stakedAndActionLockedBalanceOf to check user deposits which is zero so Attacker can deposit & stake & register to make his balance zero and repeat this and in the end reset his TopUp positions to get back his large stakes which are multiple time bigger than "max deposit Cap"
Attacker can also use this process to bypass fee penalties for early withdraw. because LiqudityPool._updateUserFeesOnDeposit() to get user current balance use stakedAndActionLockedBalanceOf() which is zero. so the value of shareExisting variable become zero and newFeeRatio will be calculated based on feeOnDeposit which can be minFee if asset is already in wallet for some time.
VIM
Add this line to TopUpActionLibrary.lockFunds() after stakerVault.transferFrom():
stakerVault.increaseActionLockedBalance(payer, amountLeft);
chase-manning (Backd) confirmed and resolved
Submitted by IllIllI
CompoundHandler.sol#L71
CompoundHandler.sol#L120
AaveHandler.sol#L53
TopUpAction.sol#L847
OpenZeppelin's safeApprove() will revert if the account already is approved and the new safeApprove() is done with a non-zero value.
function safeApprove(
IERC20 token,
address spender,
uint256 value
) internal {
// safeApprove should only be called when setting an initial allowance,
// or when resetting it to zero. To increase and decrease it, use
// 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
require(
(value == 0) || (token.allowance(address(this), spender) == 0),
"SafeERC20: approve from non-zero to non-zero allowance"
);
_callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
}OpenZeppelin/SafeERC20.sol#L45-L58
Customers cannot be topped up a second time, which will cause them to be liquidated even though they think they're protected.
There are multiple places where safeApprove() is called a second time without setting the value to zero first. The instances below are all related to topping up.
Compound-specific top-ups will fail the second time around when approving the ctoken again:
File: backd/contracts/actions/topup/handlers/CompoundHandler.sol #1
50 function topUp(
51 bytes32 account,
52 address underlying,
53 uint256 amount,
54 bytes memory extra
55 ) external override returns (bool) {
56 bool repayDebt = abi.decode(extra, (bool));
57 CToken ctoken = cTokenRegistry.fetchCToken(underlying);
58 uint256 initialTokens = ctoken.balanceOf(address(this));
59
60 address addr = account.addr();
61
62 if (repayDebt) {
63 amount -= _repayAnyDebt(addr, underlying, amount, ctoken);
64 if (amount == 0) return true;
65 }
66
67 uint256 err;
68 if (underlying == address(0)) {
69 err = ctoken.mint{value: amount}(amount);
70 } else {
71 IERC20(underlying).safeApprove(address(ctoken), amount);Compound-specific top-ups will also fail when trying to repay debt:
File: backd/contracts/actions/topup/handlers/CompoundHandler.sol #2
62 if (repayDebt) {
63 amount -= _repayAnyDebt(addr, underlying, amount, ctoken);
64 if (amount == 0) return true;
65 }Aave-specific top-ups will fail for the lendingPool:
File: backd/contracts/actions/topup/handlers/AaveHandler.sol #3
36 function topUp(
37 bytes32 account,
38 address underlying,
39 uint256 amount,
40 bytes memory extra
41 ) external override returns (bool) {
42 bool repayDebt = abi.decode(extra, (bool));
43 if (underlying == address(0)) {
44 weth.deposit{value: amount}();
45 underlying = address(weth);
46 }
47
48 address addr = account.addr();
49
50 DataTypes.ReserveData memory reserve = lendingPool.getReserveData(underlying);
51 require(reserve.aTokenAddress != address(0), Error.UNDERLYING_NOT_SUPPORTED);
52
53 IERC20(underlying).safeApprove(address(lendingPool), amount);The TopUpAction itself fails for the feeHandler:
File: backd/contracts/actions/topup/TopUpAction.sol #4
840 function _payFees(
841 address payer,
842 address beneficiary,
843 uint256 feeAmount,
844 address depositToken
845 ) internal {
846 address feeHandler = getFeeHandler();
847 IERC20(depositToken).safeApprove(feeHandler, feeAmount);I've filed the other less-severe instances as a separate medium-severity issue, and flagged the remaining low-severity instances in my QA report.
Always do safeApprove(0) if the allowance is being changed, or use safeIncreaseAllowance().
chase-manning (Backd) confirmed and resolved
Submitted by Dravee, also found by antonttc, berndartmueller, cccz, danb, horsefacts, hyh, IllIllI, MaratCerby, pauliax, rayn, UnusualTurtle, WatchPug, and wuwe1
This is a classic Code4rena issue:
- code-423n4/2021-04-meebits-findings#2
- code-423n4/2021-10-tally-findings#20
- code-423n4/2022-01-openleverage-findings#75
The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:
- The claimer smart contract does not implement a payable function.
- The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
- The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.
Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.
backd/contracts/pool/EthPool.sol:
30: to.transfer(amount);
backd/contracts/strategies/BkdEthCvx.sol:
77: payable(vault).transfer(amount);
93: payable(vault).transfer(amount);
117: payable(vault).transfer(underlyingBalance);
backd/contracts/vault/EthVault.sol:
29: payable(to).transfer(amount);
37: payable(addressProvider.getTreasury()).transfer(amount);
backd/contracts/vault/VaultReserve.sol:
81: payable(msg.sender).transfer(amount);I recommend using call() instead of transfer().
chase-manning (Backd) confirmed and resolved
Sponsor confirmed. Judging this as Medium Risk.
Submitted by hubble, also found by antonttc, csanuragjain, gs8nrv, rayn, reassor, and TrungOre
RoleManager.sol#L43-L46
RoleManager.sol#L115-L128
The impact of this vulnerability, i.e., losing all governance control is very High.
There is a possibility, due to a corner case as described below.
Contract : RoleManager.sol
Function : renounceGovernance()
Step 0:
Let current governance role given to = CURRENT_GOV_ADDRESS
so, getRoleMemberCount() for "governance" role will return = 1
Step 1: Add a new address say ALICE to governance role, by addGovernor(ALICE)
now, ALICE also has governace role, and getRoleMemberCount() for "governance" role will return = 2
Step 2: Assume that ALICE renounces governance role, by renounceGovernance()
now, ALICE does not have governance role, but getRoleMemberCount() for "governance" role will return = 2, due to a BUG
Step 3: In some distant future, if there is a compromise of CURRENT_GOV_ADDRESS keys or due to some other reason,
its decided to revoke governance role for CURRENT_GOV_ADDRESS via renounceGovernance(), and the command succeeds
It can be assumed that since getRoleMemberCount() for "governance" role returns = 2, at least there is one other active governor address.
But now, CURRENT_GOV_ADDRESS does not have governance role, and the total governance control on the protocol is lost my mistake.
getRoleMemberCount() currently returns _roleMembers[role].length();
It should return the count only for _roles[role].members[account] = true;
Its recommended to add a new function to know who are the active members for any role,
like getRoleMembers(bytes32 role) returning address account.
chase-manning (Backd) confirmed
gzeon (judge) decreased severity to Medium
[M-03] Lack of safeApprove(0) prevents some registrations, and the changing of stakers and LP tokens
Submitted by IllIllI, also found by defsec and Dravee
TopUpAction.sol#L50
LiquidityPool.sol#L721
OpenZeppelin's safeApprove() will revert if the account already is approved and the new safeApprove() is done with a non-zero value
function safeApprove(
IERC20 token,
address spender,
uint256 value
) internal {
// safeApprove should only be called when setting an initial allowance,
// or when resetting it to zero. To increase and decrease it, use
// 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
require(
(value == 0) || (token.allowance(address(this), spender) == 0),
"SafeERC20: approve from non-zero to non-zero allowance"
);
_callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
}OpenZeppelin/SafeERC20.sol#L45-L58
Customers can be prevented from register()ing the same token/stakerVaultAddress as another customer; and once changed away from, stakers and lptokens can't be used in the future.
There are multiple places where safeApprove() is called a second time without setting the value to zero first.
register() calls lockFunds() for each user registration, and since users will use the same tokens and staker vaults, the second user's register() call will fail:
File: backd/contracts/actions/topup/TopUpAction.sol #1
36 function lockFunds(
37 address stakerVaultAddress,
38 address payer,
39 address token,
40 uint256 lockAmount,
41 uint256 depositAmount
42 ) external {
43 uint256 amountLeft = lockAmount;
44 IStakerVault stakerVault = IStakerVault(stakerVaultAddress);
45
46 // stake deposit amount
47 if (depositAmount > 0) {
48 depositAmount = depositAmount > amountLeft ? amountLeft : depositAmount;
49 IERC20(token).safeTransferFrom(payer, address(this), depositAmount);
50 IERC20(token).safeApprove(stakerVaultAddress, depositAmount);The changing of either the staker or an lp token is behind a time-lock, and once the time has passed, the changed variables rely on this function:
File: backd/contracts/pool/LiquidityPool.sol #2
717 function _approveStakerVaultSpendingLpTokens() internal {
718 address staker_ = address(staker);
719 address lpToken_ = address(lpToken);
720 if (staker_ == address(0) || lpToken_ == address(0)) return;
721 IERC20(lpToken_).safeApprove(staker_, type(uint256).max);
722 }If a bug is found in a new staker or lpToken and the governor wishes to change back to the old one(s), the governor will have to wait for the time-lock delay only to find out that the old value(s) cause the code to revert.
I've filed the other more-severe instances as a separate high-severity issue, and flagged the remaining low-severity instances in my QA report.
Always do safeApprove(0) if the allowance is being changed, or use safeIncreaseAllowance().
chase-manning (Backd) confirmed
It should be noted that the second example referring to
_approveStakerVaultSpendingLpTokens()is not an issue. This is neither a member variable that can be updated nor is it behind a time lock. Both thestakerandlpTokencan only be set once and hence thesafeApprovein the aforementioned function can only be called once.
chase-manning (Backd) resolved resolved
[M-04] CvxCrvRewardsLocker implements a swap without a slippage check that can result in a loss of funds through MEV
Submitted by Ruhum
The CvxCrvRewardsLocker contract swaps tokens through the CRV cvxCRV pool. But, it doesn't use any slippage checks. The swap is at risk of being frontrun / sandwiched which will result in a loss of funds.
Since MEV is very prominent I think the chance of that happening is pretty high.
Here's the swap: CvxCrvRewardsLocker.sol#L247-L252.
Use a proper value for minOut instead of 0.
chase-manning (Backd) confirmed
gzeon (judge) decreased severity to Medium and commented:
According to C4 Judging criteria:
Unless there is something uniquely novel created by combining vectors, most submissions regarding vulnerabilities that are inherent to a particular system or the Ethereum network as a whole should be considered QA. Examples of such vulnerabilities include front running, sandwich attacks, and MEV.
However since there is a configurable
minOutthat is deliberately set to 0, this seems to be a valid issue. I am judging this as Medium Risk.
Submitted by cccz, also found by 0x1f8b, 0xDjango, 0xkatana, berndartmueller, defsec, Dravee, horsefacts, hyh, IllIllI, kenta, rayn, reassor, sorrynotsorry, and WatchPug
On ChainlinkOracleProvider.sol and ChainlinkUsdWrapper.sol , we are using latestRoundData, but there is no check if the return value indicates stale data.
function _ethPrice() private view returns (int256) {
(, int256 answer, , , ) = _ethOracle.latestRoundData();
return answer;
}
...
function getPriceUSD(address asset) public view override returns (uint256) {
address feed = feeds[asset];
require(feed != address(0), Error.ASSET_NOT_SUPPORTED);
(, int256 answer, , uint256 updatedAt, ) = AggregatorV2V3Interface(feed).latestRoundData();
require(block.timestamp <= updatedAt + stalePriceDelay, Error.STALE_PRICE);
require(answer >= 0, Error.NEGATIVE_PRICE);
uint256 price = uint256(answer);
uint8 decimals = AggregatorV2V3Interface(feed).decimals();
return price.scaleFrom(decimals);
}This could lead to stale prices according to the Chainlink documentation:
https://docs.chain.link/docs/historical-price-data/#historical-rounds
https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round
ChainlinkOracleProvider.sol#L55
ChainlinkUsdWrapper.sol#L64
function _ethPrice() private view returns (int256) {
(uint80 roundID, int256 answer, , uint256 timestamp, uint80 answeredInRound) = _ethOracle.latestRoundData();
require(answeredInRound >= roundID, "Stale price");
require(timestamp != 0,"Round not complete");
require(answer > 0,"Chainlink answer reporting 0");
return answer;
}
...
function getPriceUSD(address asset) public view override returns (uint256) {
address feed = feeds[asset];
require(feed != address(0), Error.ASSET_NOT_SUPPORTED);
(uint80 roundID, int256 answer, , uint256 updatedAt, uint80 answeredInRound) = AggregatorV2V3Interface(feed).latestRoundData();
require(answeredInRound >= roundID, "Stale price");
require(answer > 0," Error.NEGATIVE_PRICE");
require(block.timestamp <= updatedAt + stalePriceDelay, Error.STALE_PRICE);
uint256 price = uint256(answer);
uint8 decimals = AggregatorV2V3Interface(feed).decimals();
return price.scaleFrom(decimals);
}chase-manning (Backd) confirmed and resolved
Submitted by shenwilly, also found by wuwe1
When ERC777 token is used as the underlying token for a LiquidityPool, a depositor can reenter depositFor and bypass the depositCap requirement check, resulting in higher total deposit than intended by governance.
- An empty ERC777 liquidity pool is capped at 1.000 token.
- Alice deposits 1.000 token. Before the token is actually sent to the contract,
tokensToSendERC777 hook is called and Alice reentersdepositFor. - As the previous deposit hasn't been taken into account, the reentrancy passes the
depositCapcheck. - Pool has 2.000 token now, despite the 1.000 deposit cap.
Add reentrancy guards to depositFor.
chase-manning (Backd) confirmed and resolved
Submitted by fatherOfBlocks, also found by shenwilly
StrategySwapper.sol#L38-L43
StrategySwapper.sol#L109-L114
In the setSlippageTolerance(L119) method you have certain requirements to set slippageTolerance, but in the constructor you don't.
I would add the corresponding validations to the constructor.
chase-manning (Backd) confirmed and resolved
Submitted by shenwilly, also found by pauliax, StyxRave, and WatchPug
StrategySwapper.sol#L287-L289
StrategySwapper.sol#L318-L320
StrategySwapper.sol#L335-L337
In StrategySwapper, swapping from or to tokens with decimals higher than 18 will always revert. This will cause inabilities for strategies to harvest rewards.
L288 will revert when token_ has higher than 18 decimals.
return 10**(18 - IERC20Full(token_).decimals());
Consider modifying how _decimalMultiplier works so it could handle tokens with higher than 18 decimals.
Update the calculation of _minTokenAmountOut and _minWethAmountOut to account when decimals are higher/lower than 18.
chase-manning (Backd) confirmed and resolved
Submitted by shenwilly
Depositors won't be able to transfer or redeem funds temporarily.
The problem is caused by the implementation of LiquidityPool.getNewCurrentFees:
function getNewCurrentFees(
uint256 timeToWait,
uint256 lastActionTimestamp,
uint256 feeRatio
) public view returns (uint256) {
uint256 timeElapsed = _getTime() - lastActionTimestamp;
uint256 minFeePercentage = getMinWithdrawalFee();
if (timeElapsed >= timeToWait) {
return minFeePercentage;
}
uint256 elapsedShare = timeElapsed.scaledDiv(timeToWait);
return feeRatio - (feeRatio - minFeePercentage).scaledMul(elapsedShare);
}The last line requires the current feeRatio to be higher than minFeePercentage or the function will revert. When this condition is broken, some critical functions such as transferring tokens and redeeming will be unusable. Affected users need to wait until enough time has elapsed and getNewCurrentFees returns minFeePercentage on L691.
This could happen if governance changes the MinWithdrawalFee to be higher than a user's feeRatio.
- Initial
MinWithdrawalFeeis set to 0,MaxWithdrawalFeeis set to 0.03e18. - Alice deposits fund and receives LP token. Alice's
feeRatiois now set to 0.03e18 (the currentMaxWithdrawalFee). - Governance changes
MaxWithdrawalFeeto0.05e18andMinWithdrawalFeeto0.04e18. minFeePercentageis now higher than Alice'sfeeRatioand she can't transfer nor redeem the LP token untiltimeElapsed >= timeToWait.
Add a new condition in getNewCurrentFees L690 to account for this case:
if (timeElapsed >= timeToWait || minFeePercentage > feeRatio) {
return minFeePercentage;
}chase-manning (Backd) confirmed and resolved
Submitted by 0xDjango
The _updateUserFeesOnDeposit() function in LiquidityPool.sol is used to update a user's withdrawal fees after an action such as deposit, transfer in, etc. The withdrawal fee decays toward a minimum withdrawal fee over a period of 1 or 2 weeks (discussed with developer). Since anyone can transfer lp tokens to any user, a griefer can transfer 1 wei of lp tokens to another user to reset their lastActionTimestamp used in the withdrawal fee calculation.
The developers nicely weight the updated withdrawal fee by taking the original balance/original fee vs the added balance/added fee. The attacker will only be able to extend the runway of the withdrawal fee cooldown by resetting the lastActionTimestamp for future calculations. Example below:
Assumptions:
- MinWithdrawalFee = 0% //For easy math
- MaxWithdrawalFee = 10%
- timeToWait = 2 weeks
- User A has
100 weiof shares - User A waits 1 week (Current withdrawal fee = 5%)
- User B deposits, receives
1 weiof shares, current withdrawal fee = 10% - User B immediately transfers
1 weiof shares to User A
Based on the formula to calculated User A's new feeRatio:
uint256 newFeeRatio = shareExisting.scaledMul(newCurrentFeeRatio) +
shareAdded.scaledMul(feeOnDeposit);In reality, User A's withdrawal fee will only increase by a negligible amount since the shares added were very small in proportion to the original shares. We can assume user A's current withdrawal fee is still 5%.
The issue is that the function then reset's User A's lastActionTimestamp to the current time. This means that User A will have to wait the maximum 2 weeks for the withdrawal fee to reduce from 5% to 0%. Effectively the cooldown runway is the same length as the original runway length, so the decay down to 0% will take twice as long.
meta.lastActionTimestamp = uint64(_getTime());
Instead of resetting lastActionTimestamp to the current time, scale it the same way the feeRatio is scaled. I understand that this would technically not be the timestamp of the last action, so the variable would probably need to be renamed.
chase-manning (Backd) confirmed and resolved
Submitted by 0x52
TopUpAction.sol#L154
TopUpAction.sol#L187
The default swap slippage of 5% allows malicious keepers to sandwich attack topup. Additionally, up to 40% (_MIN_SWAPPER_SLIPPAGE) slippage allows malicious owner to sandwich huge amounts from topup
Keeper can bundle swaps before and after topup to sandwich topup action, in fact it's actually in their best interest to do so.
Allow user to specify max swap slippage when creating topup similar to how it's specified on uniswap or sushiswap to block attacks from both keepers and owners.
chase-manning (Backd) confirmed and resolved
According to C4 Judging criteria
Unless there is something uniquely novel created by combining vectors, most submissions regarding vulnerabilities that are inherent to a particular system or the Ethereum network as a whole should be considered QA. Examples of such vulnerabilities include front running, sandwich attacks, and MEV.
However since Backd use keeper to run topup transactions, which presumably are bots and smart contracts that can fetch onchain price directly. A large (5% default, up to 40%) seems excessive and can lead to user losing fund. Judging this as Medium Risk.
[M-12] CompoundHandler#topUp() Using the wrong function selector makes native token topUp() always revert
Submitted by WatchPug
compound-finance/CEther.sol#L44-L47
function mint() external payable {
(uint err,) = mintInternal(msg.value);
requireNoError(err, "mint failed");
}mint() for native cToken (CEther) does not have any parameters, as the Function Selector is based on the function name with the parenthesised list of parameter types, when you add a nonexisting parameter, the Function Selector will be incorrect.
function mint(uint256 mintAmount) external payable virtual returns (uint256);The current implementation uses the same CToken interface for both CEther and CErc20 in topUp(), and function mint(uint256 mintAmount) is a nonexisting function for CEther.
As a result, the native token topUp() always revert.
CToken ctoken = cTokenRegistry.fetchCToken(underlying);
uint256 initialTokens = ctoken.balanceOf(address(this));
address addr = account.addr();
if (repayDebt) {
amount -= _repayAnyDebt(addr, underlying, amount, ctoken);
if (amount == 0) return true;
}
uint256 err;
if (underlying == address(0)) {
err = ctoken.mint{value: amount}(amount);
}See also:
samwerner (Backd) confirmed and resolved
[M-13] CEthInterface#repayBorrowBehalf() reading non-existing returns makes _repayAnyDebt() with CEther always revert
Submitted by WatchPug
CTokenInterfaces.sol#L355-L358
function repayBorrowBehalf(address borrower, uint256 repayAmount)
external
payable
returns (uint256);repayBorrowBehalf() for native cToken (CEther) will return nothing, while the current CEthInterface interface defines the returns as (uint256).
As a result, ether.repayBorrowBehalf() will always revert
CEther cether = CEther(address(ctoken));
err = cether.repayBorrowBehalf{value: debt}(account);Ref:
| method | CEther | CErc20 |
|---|---|---|
| mint() | revert | error code |
| redeem() | error code | error code |
| repayBorrow() | revert | error code |
| repayBorrowBehalf() | revert | error code |
- Compound cToken Repay Borrow Behalf doc
- Compound CEther.repayBorrowBehalf()
- Compound CErc20.repayBorrowBehalf()
chase-manning (Backd) confirmed
[M-14] CEthInterface#mint() reading non-existing returns makes topUp() with native token always revert
Submitted by WatchPug
function mint() external payable returns (uint256);mint() for native cToken (CEther) will return nothing, while the current CEthInterface interface defines the returns as (uint256).
In the current implementation, the interface for CToken is used for both CEther and CErc20.
As a result, the transaction will revert with the error: function returned an unexpected amount of data when topUp() with the native token (ETH).
CToken ctoken = cTokenRegistry.fetchCToken(underlying);
uint256 initialTokens = ctoken.balanceOf(address(this));
address addr = account.addr();
if (repayDebt) {
amount -= _repayAnyDebt(addr, underlying, amount, ctoken);
if (amount == 0) return true;
}
uint256 err;
if (underlying == address(0)) {
err = ctoken.mint{value: amount}(amount);
}Ref:
| method | CEther | CErc20 |
|---|---|---|
| mint() | revert | error code |
| redeem() | error code | error code |
| repayBorrow() | revert | error code |
| repayBorrowBehalf() | revert | error code |
chase-manning (Backd) confirmed
Submitted by sseefried
A Staker -- that has their top-up position removed after execute is called by a Keeper -- can always cause the transaction to revert. They can do this by deploying a smart contract to the payer address that has implemented a receive() function that calls revert(). The revert will be triggered by the following lines in execute
if (vars.removePosition) {
gasBank.withdrawUnused(payer);
}This will consume some gas from the keeper while preventing them accruing any rewards for performing the top-up action.
I have implemented a PoC in a fork of the contest repo. The attacker's contract can be found here.
To prevent this denial of service attack some way of blacklisting badly behaved Stakers should be added.
chase-manning (Backd) confirmed
For this contest, 39 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: horsefacts, sseefried, robee, defsec, hubble, 0xDjango, sorrynotsorry, berndartmueller, Dravee, joestakey, StyxRave, 0xkatana, csanuragjain, dipp, hake, kebabsec, pauliax, peritoflores, securerodd, z3s, 0v3rf10w, 0x52, catchup, fatherOfBlocks, Funen, jayjonah8, Kenshin, kenta, m4rio_eth, oyc_109, rayn, remora, Ruhum, simon135, Tadashi, TerrierLover, TrungOre, and antonttc.
Vulnerability details:
_lastWithdrawal[vault] will always be zero for new vaults, so the check is for 0 + minWithdrawalDelay which will always be less than block.timestamp
File: backd/contracts/vault/VaultReserve.sol #1
102 function canWithdraw(address vault) public view returns (bool) {
103 return block.timestamp >= _lastWithdrawal[vault] + minWithdrawalDelay;Unlike CompoundHandler, AaveHandler does not extend BaseHandler, which will cause storage problems in future versions
File: backd/contracts/actions/topup/handlers/AaveHandler.sol #1
15 contract AaveHandler is ITopUpHandler {If the intention is for the Ether to be used, the function should call another function, otherwise it should revert
File: contracts/actions/topup/TopUpAction.sol #1
176 receive() external payable {
177 // solhint-disable-previous-line no-empty-blocks
178 }File: contracts/pool/EthPool.sol #2
10 receive() external payable {}File: contracts/strategies/BkdEthCvx.sol #3
46 receive() external payable {}File: contracts/strategies/StrategySwapper.sol #4
45 receive() external payable {}File: contracts/vault/EthVault.sol #5
13 receive() external payable {}If the initializer is not executed in the same transaction as the constructor, a malicious user can front-run the initialize() call, forcing the contract to be redeployed. Most other initializers in this project are protected, but this one appears not to be.
File: backd/contracts/AddressProvider.sol #1
53 function initialize(address roleManager) external initializer {
54 AddressProviderMeta.Meta memory meta = AddressProviderMeta.Meta(true, true);
55 _addressKeyMetas.set(AddressProviderKeys._ROLE_MANAGER_KEY, meta.toUInt());
56 _setConfig(AddressProviderKeys._ROLE_MANAGER_KEY, roleManager);
57 }Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance()
See original submission for instances.
File: contracts/actions/topup/TopUpActionFeeHandler.sol #1
55 actionContract = _actionContract;File: contracts/CvxCrvRewardsLocker.sol #2
151 treasury = _treasury;File: contracts/StakerVault.sol #3
66 token = _token;File: contracts/strategies/ConvexStrategyBase.sol #4
100 vault = vault_;File: contracts/strategies/ConvexStrategyBase.sol #5
101 _strategist = strategist_;File: contracts/strategies/ConvexStrategyBase.sol #6
182 communityReserve = _communityReserve;File: contracts/strategies/ConvexStrategyBase.sol #7
261 _strategist = strategist_;[L-07] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
File: contracts/actions/topup/handlers/CTokenRegistry.sol #1
67 keccak256(abi.encodePacked(ctoken.symbol())) == keccak256(abi.encodePacked("cETH"))The use of payable.transfer() is heavily frowned upon because it can lead to the locking of funds. The transfer() call requires that the recipient has a payable callback, only provides 2300 gas for its operation. This means the following cases can cause the transfer to fail:
- The contract does not have a
payablecallback - The contract's
payablecallback spends more than 2300 gas (which is only enough to emit something) - The contract is called through a proxy which itself uses up the 2300 gas
File: backd/contracts/vault/VaultReserve.sol #1
81 payable(msg.sender).transfer(amount);uses the onlyVault modifier, and vaults currently have empty payable callbacks, so they don't currently revert
https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/vault/VaultReserve.sol#L81
File: backd/contracts/vault/EthVault.sol #2
29 payable(to).transfer(amount);uses the onlyPoolOrGovernance modifier, and pools currently have an empty payable callback, so they don't currently rever. Governance is currently deployed and not seeing issues, so presumably it also has an empty payable callback
https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/vault/EthVault.sol#L29
File: backd/contracts/vault/EthVault.sol #3
37 payable(addressProvider.getTreasury()).transfer(amount);the treasury is currently deployed and not seeing issues, so presumably it also has an empty payable callback
https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/vault/EthVault.sol#L37
File: backd/contracts/strategies/BkdEthCvx.sol #4
77 payable(vault).transfer(amount);vaults currently have an empty payable callback
https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/strategies/BkdEthCvx.sol#L77
File: backd/contracts/strategies/BkdEthCvx.sol #5
93 payable(vault).transfer(amount);vaults currently have an empty payable callback
https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/strategies/BkdEthCvx.sol#L93
File: backd/contracts/strategies/BkdEthCvx.sol #6
117 payable(vault).transfer(underlyingBalance);vaults currently have an empty payable callback
https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/strategies/BkdEthCvx.sol#L117
[L-09] Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions
See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
File: contracts/LpToken.sol #1
10 contract LpToken is ILpToken, ERC20Upgradeable {In the example below, a + b may overflow even though the division that comes later would prevent it. This particular case can be prevented by doing (a & b) + (a ^ b) / b. There are other functions with similar issues. See this library for ways of doing math without this sort of issue.
File: backd/libraries/ScaledMath.sol #1
40 function divRoundUp(uint256 a, uint256 b) internal pure returns (uint256) {
41 return (a + b - 1) / b;
42 }These functions have the ability to bypass the timelocks of every setting. No contract besides the Preparable contract itself should need to call these functions, and having them available will lead to exploits. The contracts that currently call _setConfig() in their constructors should be given a new function _initConfig() for this purpose. The Vault calls some of these functions as well, and should be changed to manually inspect the deadline rather than mucking with the internals, which is error-prone. The mappings should also be made private, and there should be public getters to read their values
File: backd/contracts/utils/Preparable.sol #1
115 /**
116 * @notice Execute uint256 config update (with time delay enforced).
117 * @dev Needs to be called after the update was prepared. Fails if called before time delay is met.
118 * @return New value.
119 */
120 function _executeUInt256(bytes32 key) internal returns (uint256) {
121 _executeDeadline(key);
122 uint256 newValue = pendingUInts256[key];
123 _setConfig(key, newValue);
124 return newValue;
125 }
126
127 /**
128 * @notice Execute address config update (with time delay enforced).
129 * @dev Needs to be called after the update was prepared. Fails if called before time delay is met.
130 * @return New value.
131 */
132 function _executeAddress(bytes32 key) internal returns (address) {
133 _executeDeadline(key);
134 address newValue = pendingAddresses[key];
135 _setConfig(key, newValue);
136 return newValue;
137 }Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment
File: contracts/actions/topup/TopUpAction.sol #1
713 // TODO: add constant gas consumed for transfer and tx prologueFile: contracts/strategies/ConvexStrategyBase.sol #2
4 // TODO Add validation of curve poolsFile: contracts/strategies/ConvexStrategyBase.sol #3
5 // TODO Test validationFile: backd/contracts/vault/VaultReserve.sol #1
50 if (token == address(0)) {
51 require(msg.value == amount, Error.INVALID_AMOUNT);
52 _balances[msg.sender][token] += msg.value;
53 return true;
54 }
55 uint256 balance = IERC20(token).balanceOf(address(this));After the if-statement there should be a require(0 == msg.value) to ensure no Ether is being used when updating ERC20 balances. This is non-critical since the function has the onlyVault modifier, and presumably vaults would be coded never to deposit Ether to ERC20 tokens
https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/vault/VaultReserve.sol#L50-L55
File: contracts/pool/PoolFactory.sol #1
216 return addrs;Contracts are allowed to override their parents' functions and change the visibility from external to public.
File: contracts/actions/topup/TopUpAction.sol #1
742 function prepareTopUpHandler(bytes32 protocol, address newHandler)
743 public
744 onlyGovernance
745 returns (bool)File: contracts/CvxCrvRewardsLocker.sol #2
222 function withdraw(address token, uint256 amount) public onlyGovernance returns (bool) {See original submission for instances.
[N-07] Large multiples of ten should use scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000), for readability
File: contracts/utils/CvxMintAmount.sol #1
7 uint256 private constant _CLIFF_SIZE = 100000 * 1e18; //new cliff every 100,000 tokensFile: contracts/utils/CvxMintAmount.sol #2
9 uint256 private constant _MAX_SUPPLY = 100000000 * 1e18; //100 mil max supplyUse a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(,)
File: contracts/actions/topup/handlers/CTokenRegistry.sol #1
2 pragma solidity 0.8.9;File: contracts/actions/topup/TopUpActionFeeHandler.sol #2
2 pragma solidity 0.8.9;File: contracts/actions/topup/TopUpAction.sol #3
2 pragma solidity 0.8.9;Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.
See original submission for instances.
Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file
File: contracts/utils/CvxMintAmount.sol #1
8 uint256 private constant _CLIFF_COUNT = 1000; // 1,000 cliffsFile: contracts/utils/CvxMintAmount.sol #2
11 IERC20(address(0x4e3FBD56CD56c3e72c1403e103b45Db9da5B9D2B)); // CVX TokenSee original submission for instances.
File: contracts/access/Authorization.sol (various lines) #1
File: contracts/access/RoleManager.sol (various lines) #2
File: contracts/oracles/ChainlinkUsdWrapper.sol (various lines) #3
File: contracts/oracles/OracleProviderExtensions.sol (various lines) #4
File: contracts/pool/Erc20Pool.sol (various lines) #5
File: contracts/pool/EthPool.sol (various lines) #6
File: contracts/utils/CvxMintAmount.sol (various lines) #7
File: contracts/vault/Erc20Vault.sol (various lines) #8
File: contracts/vault/EthVault.sol (various lines) #9
File: libraries/AddressProviderMeta.sol (various lines) #10
File: libraries/Errors.sol (various lines) #11
See original submission for instances.
Each event should use three indexed fields if there are three or more fields
See original submission for instances.
chase-manning (Backd) resolved and commented:
I consider this report to be of particularly high quality.
Nice submission, warden covered basically all the low risk and non-critical issues. Would be nice if there was an index.
For this contest, 35 reports were submitted by wardens detailing gas optimizations. The report highlighted below by joestakey received the top score from the judge.
The following wardens also submitted reports: Tomio, IllIllI, Dravee, catchup, defsec, securerodd, 0xkatana, kenta, robee, slywaters, sorrynotsorry, 0v3rf10w, 0x1f8b, 0x4non, 0xNazgul, fatherOfBlocks, Funen, NoamYakov, pauliax, rfa, saian, TerrierLover, WatchPug, MaratCerby, 0xDjango, 0xmint, hake, horsefacts, oyc_109, rayn, simon135, Tadashi, tin537, and z3s.
Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.
In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length in memory can yield significant gas savings if the array length is high.
Instances include:
scope: topUp()
wethis read twice
AaveHandler.sol:44
AaveHandler.sol:45
lendingPoolis read 4 times
AaveHandler.sol:50
AaveHandler.sol:53
AaveHandler.sol:60
AaveHandler.sol:65
scope: _getAccountBorrowsAndSupply()
comptrolleris read (2 +assets.length) times. Number of read depends on the length ofassetsas it is in a for loop
CompoundHandler.sol:132
CompoundHandler.sol:134
CompoundHandler.sol:142
scope: _isCTokenUsable()
comptrolleris read 3 times
CTokenRegistry.sol:77
CTokenRegistry.sol:79
CTokenRegistry.sol:80
scope: resetPosition()
addressProvideris read twice
TopUpAction.sol:284
TopUpAction.sol:295
scope: execute()
addressProvideris read 3 times
TopUpAction.sol:562
TopUpAction.sol:604
TopUpAction.sol:632
scope: _withdraw()
vaultis read twice
BkdEthCvx.sol:77
BkdEthCvx.sol:93
scope: _withdraw()
vaultis read twice
BkdTriHopCvx.sol:175
BkdTriHopCvx.sol:201
scope: addRewardToken()
_strategySwapperis read twice
ConvexStrategyBase.sol:279
ConvexStrategyBase.sol:280
scope: harvestable()
crvCommunityReserveShareis read twice
ConvexStrategyBase.sol:307
ConvexStrategyBase.sol:311
_rewardTokens.length()is read_rewardTokens.length()times. Number of read depends on the length of_rewardsTokensas it is in a for loop
ConvexStrategyBase.sol:313
scope: _harvest()
_rewardTokens.length()is read_rewardTokens.length()times. Number of read depends on the length of_rewardsTokensas it is in a for loop
ConvexStrategyBase.sol:380
scope: _sendCommunityReserveShare()
cvxCommunityReserveShareis read twice
ConvexStrategyBase.sol:398
ConvexStrategyBase.sol:409
scope: _handleExcessDebt()
reserveis read 3 times
Vault.sol:645
Vault.sol:648
Vault.sol:649
scope: _handleExcessDebt()
totalDebtis read twice
Vault.sol:657
Vault.sol:658
scope: stakeFor()
tokenis read 4 times
Vault.sol:324
Vault.sol:331
Vault.sol:338
Vault.sol:339
scope: unStakeFor()
tokenis read 4 times
Vault.sol:365
Vault.sol:376
Vault.sol:382
Vault.sol:384
cache these storage variables in memory
If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.
Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.
Instances include:
scope: hasAnyRole()
RoleManager.sol:73: bytes32[] memory roles
scope: topUp()
AaveHandler.sol:40: bytes memory extra
scope: topUp()
CompoundHandler.sol:54: bytes memory extra
scope: getHealthFactor()
TopUpAction.sol:760: bytes memory extra
scope: canExecute()
TopUpKeeperHelper.sol:108: ITopUpAction.RecordKey memory key
scope: _canExecute()
TopUpKeeperHelper.sol:131: ITopUpAction.RecordWithMeta memory position
scope: _positionToTopup()
TopUpKeeperHelper.sol:145: ITopUpAction.RecordWithMeta memory position
scope: _shortenTopups()
TopUpKeeperHelper.sol:159: TopupData[] memory topups
scope: initialize()
Erc20Pool.sol:15: string memory name_
scope: initialize()
EthPool.sol:13: string memory name_
scope: initialize()
LiquidityPool.sol:702: string memory name_
scope: initialize()
LpToken.sol:29: string memory name_
LpToken.sol:30: string memory symbol_
Replace memory with calldata
>0 is less gas efficient than !0 if you enable the optimizer at 10k AND you’re in a require statement.
Detailed explanation with the opcodes here
Instances include:
scope: register()
TopUpAction.sol:210
scope: execute()
TopUpAction.sol:554
scope: claimKeeperFeesForPool()
TopUpActionFeeHandler.sol:123
scope: updateDepositCap()
LiquidityPool.sol:401
scope: calcRedeem()
LiquidityPool.sol:471
LiquidityPool.sol:473
scope: redeem()
LiquidityPool.sol:549
scope: withdrawFromReserve()
Vault.sol:164
scope: depositFees()
BkdLocker.sol:90
BkdLocker.sol:91
BkdLocker.sol:136
Replace > 0 with !0
In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.
Using strict comparison operators hence saves gas
Instances include:
TopUpAction.sol:61
TopUpAction.sol:212
TopUpAction.sol:224
TopUpAction.sol:328
TopUpAction.sol:360
TopUpAction.sol:361
TopUpAction.sol:500
TopUpAction.sol:501
TopUpAction.sol:576
TopUpAction.sol:584
TopUpAction.sol:724
TopUpActionFeeHandler.sol:54
TopUpActionFeeHandler.sol:151
TopUpActionFeeHandler.sol:163
TopUpActionFeeHandler.sol:196
TopUpActionFeeHandler.sol:208
ChainLinkOracleProvider.sol:41
ChainLinkOracleProvider.sol:57
EthPool.sol:442
EthPool.sol:208
EthPool.sol:518
EthPool.sol:525
EthPool.sol:551
EthPool.sol:562
EthPool.sol:690
EthPool.sol:811
EthPool.sol:812
BkdEthCvx.sol:76
BkdTriHopCvx.sol:174
ConvexStrategyBase.sol:197
ConvexStrategyBase.sol:214
StrategySwapper.sol:110
CvxMintAmount.sol:21
Preparable.sol:29
Preparable.sol:110
Vault.sol:88
Vault.sol:89
Vault.sol:90
Vault.sol:167
Vault.sol:264
Vault.sol:323
Vault.sol:392
Vault.sol:437
Vault.sol:482
Vault.sol:712
Vault.sol:763
VaultReserve.sol:59
VaultReserve.sol:75
VaultReserve.sol:103
BkdLocker.sol:119
BkdLocker.sol:140
BkdLocker.sol:281
Controller:98
CvxCrvRewardsLocker:84
GasBank:68
GasBank:76
StakerVault:107
StakerVault:150
StakerVault:153
StakerVault:324
StakerVault:368
StakerVault:371
Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable
example:
-require(maxFee >= minFee, Error.INVALID_AMOUNT);
+require(maxFee > minFee - 1, Error.INVALID_AMOUNT);
When the comparison is with a constant storage variable, you can also do the increment in the storage variable declaration
example:
-require(maxFee <= _MAX_WITHDRAWAL_FEE, Error.INVALID_AMOUNT)
+require(maxFee < _MAX_WITHDRAWAL_FEE_PLUS_ONE, Error.INVALID_AMOUNT)
However, when 1 is negligible compared to the variable (with is the case here as the variable is in the order of 10**16), it is not necessary to increment.
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here
Custom errors are defined using the error statement
Instances include:
RoleManager.sol:44
RoleManager.sol:110
RoleManager.sol:111
AaveHandler.sol:51
CompoundHandler.sol:74
CompoundHandler.sol:80
CompoundHandler.sol:123
CompoundHandler.sol:141
CompoundHandler.sol:148
TopUpAction.sol:67
TopUpAction.sol:98
TopUpAction.sol:185
TopUpAction.sol:209
TopUpAction.sol:210
TopUpAction.sol:211
TopUpAction.sol:212
TopUpAction.sol:213
TopUpAction.sol:217
TopUpAction.sol:218
TopUpAction.sol:224
TopUpAction.sol:282
TopUpAction.sol:328
TopUpAction.sol:359
TopUpAction.sol:546
TopUpAction.sol:553
TopUpAction.sol:554
TopUpAction.sol:560
TopUpAction.sol:575
TopUpAction.sol:583
TopUpAction.sol:676
TopUpAction.sol:723
TopUpAction.sol:928
TopUpActionFeeHandler.sol:67
TopUpActionFeeHandler.sol:68
TopUpActionFeeHandler.sol:87
TopUpActionFeeHandler.sol:123
TopUpActionFeeHandler.sol:151
TopUpActionFeeHandler.sol:161
TopUpActionFeeHandler.sol:196
TopUpActionFeeHandler.sol:206
ChainLinkOracleProvider.sol:31
ChainLinkOracleProvider.sol:41
ChainLinkOracleProvider.sol:53
ChainLinkOracleProvider.sol:57
ChainLinkOracleProvider.sol:58
Erc20Pool.sol:20
Erc20Pool.sol:30
EthPool.sol:25
EthPool.sol:26
LiquidityPool.sol:76
LiquidityPool.sol:136
LiquidityPool.sol:137
LiquidityPool.sol:155
LiquidityPool.sol:179
LiquidityPool.sol:208
LiquidityPool.sol:331
LiquidityPool.sol:333
LiquidityPool.sol:387
LiquidityPool.sol:399
LiquidityPool.sol:400
LiquidityPool.sol:401
LiquidityPool.sol:441
LiquidityPool.sol:471
LiquidityPool.sol:473
LiquidityPool.sol:517
LiquidityPool.sol:525
LiquidityPool.sol:549
LiquidityPool.sol:551
LiquidityPool.sol:562
LiquidityPool.sol:811
LiquidityPool.sol:812
PoolFactory.sol:159
PoolFactory.sol:162
PoolFactory.sol:165
PoolFactory.sol:170
PoolFactory.sol:180
PoolFactory.sol:184
BkdTriHopCvx.sol:133
BkdTriHopCvx.sol:147
ConvexStrategyBase.sol:117
ConvexStrategyBase.sol:144
ConvexStrategyBase.sol:197
ConvexStrategyBase.sol:198
ConvexStrategyBase.sol:214
ConvexStrategyBase.sol:215
ConvexStrategyBase.sol:260
ConvexStrategyBase.sol:273
StrategySwapper.sol:69
StrategySwapper.sol:110
StrategySwapper.sol:111
StrategySwapper.sol:123
StrategySwapper.sol:124
StrategySwapper.sol:139
Preparable.sol:28
Preparable.sol:29
Preparable.sol:86
Preparable.sol:98
Preparable.sol:110
Preparable.sol:111
Erc20Vault.sol:20
Vault.sol:88
Vault.sol:89
Vault.sol:90
Vault.sol:164
Vault.sol:165
Vault.sol:167
Vault.sol:194
Vault.sol:195
Vault.sol:198
Vault.sol:264
Vault.sol:392
Vault.sol:429
Vault.sol:762
VaultReserve.sol:51
VaultReserve.sol:59
VaultReserve.sol:73
VaultReserve.sol:75
AddressProvider.sol:64
AddressProvider.sol:70
AddressProvider.sol:96
AddressProvider.sol:100
AddressProvider.sol:170
AddressProvider.sol:179
AddressProvider.sol:188
AddressProvider.sol:230
AddressProvider.sol:231
AddressProvider.sol:249
AddressProvider.sol:259
AddressProvider.sol:284
AddressProvider.sol:285
AddressProvider.sol:314
AddressProvider.sol:417
AddressProvider.sol:423
BkdLocker.sol:58
BkdLocker.sol:90
BkdLocker.sol:91
BkdLocker.sol:118
BkdLocker.sol:136
BkdLocker.sol:207
Controller:32
Controller:33
Controller:80
CvxCrvRewardsLocker:83
CvxCrvRewardsLocker:135
GasBank:42
GasBank:68
GasBank:69
GasBank:76
GasBank:91
LpToken:34
StakerVault:70
StakerVault:93
StakerVault:106
StakerVault:107
StakerVault:139
StakerVault:150
StakerVault:153
StakerVault:203
StakerVault:224
StakerVault:324
StakerVault:340
StakerVault:367
StakerVault:371
SwapperRegistry:35
Replace require statements with custom errors.
If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.
Instances include:
RoleManager.sol:80
RoleManager.sol:110
RoleManager.sol:111
CompoundHandler.sol:135
CTokenRegistry.sol:61
TopUpAction.sol:188
TopUpAction.sol:452
TopUpAction.sol:456
TopUpAction.sol:479
TopUpAction.sol:506
TopUpAction.sol:891
TopUpActionKeeperHandler.sol:43
TopUpActionKeeperHandler.sol:46
TopUpActionKeeperHandler.sol:72
TopUpActionKeeperHandler.sol:93
TopUpActionKeeperHandler.sol:165
LiquidityPool.sol:483
ConvexStrategyBase.sol:313
ConvexStrategyBase.sol:380
Vault.sol:42
Vault.sol:135
Vault.sol:583
BkdLocker.sol:133
BkdLocker.sol:310
Controller:114
Controller:117
CvxCrvRewardsLocker:43
StakerVault:144
StakerVault:260
Remove explicit initialization for default values.
Prefix increments are cheaper than postfix increments.
Instances include:
RoleManager.sol:80
CompoundHandler.sol:135
CTokenRegistry.sol:61
TopUpAction.sol:188
TopUpAction.sol:456
TopUpAction.sol:479
TopUpAction.sol:506
TopUpAction.sol:891
TopUpActionKeeperHandler.sol:43
TopUpActionKeeperHandler.sol:46
TopUpActionKeeperHandler.sol:50
TopUpActionKeeperHandler.sol:72
TopUpActionKeeperHandler.sol:93
TopUpActionKeeperHandler.sol:165
ConvexStrategyBase.sol:313
ConvexStrategyBase.sol:380
BkdLocker.sol:310
Controller:117
StakerVault:260
change variable++ to ++variable
Redundant code should be avoided as it costs unnecessary gas
Instances include:
Preparable.sol:140:
address oldValue = currentAddresses[key];
currentAddresses[key] = value;
pendingAddresses[key] = address(0);
deadlines[key] = 0;
emit ConfigUpdatedAddress(key, oldValue, value);
return value;
We can update currentAddresses[key] after emitting the event to save the gas of the declaration of oldValue:
+emit ConfigUpdatedAddress(key, currentAddresses[key], value);
pendingAddresses[key] = address(0);
deadlines[key] = 0;
currentAddresses[key] = value;
return value;
see Proof of Concept for mitigation steps.
Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.
Instances include:
TopUpAction:360:
require(
newSwapperSlippage >= _MIN_SWAPPER_SLIPPAGE &&
newSwapperSlippage <= _MAX_SWAPPER_SLIPPAGE,
Error.INVALID_AMOUNT
);
ConvexStrategyBase:274:
require(
token_ != address(_CVX) && token_ != address(underlying) && token_ != address(_CRV),
Error.INVALID_TOKEN_TO_ADD
);
SwapperRegistry:35:
require(
fromToken != toToken &&
fromToken != address(0) &&
toToken != address(0) &&
newSwapper != address(0),
Error.INVALID_TOKEN_PAIR
);
Breakdown each condition in a separate require statement (though require statements should be replaced with custom errors)
require( newSwapperSlippage >=_MIN_SWAPPER_SLIPPAGE);
require(newSwapperSlippage <= _MAX_SWAPPER_SLIPPAGE,
Error.INVALID_AMOUNT)
Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs.
address type variables are each of 20 bytes size (way less than 32 bytes). However, they here take up a whole 32 bytes slot (they are contiguous).
As bool type variables are of size 1 byte, there's a slot here that can get saved by moving them closer to an address
Instances include:
VaultStorage.sol:11:
address public pool;
uint256 public totalDebt;
bool public strategyActive;
Place strategyActive after pool to save one storage slot
address public pool;
+bool public strategyActive;
uint256 public totalDebt;
The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.
if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas
Instances include:
LiquidityPool.sol:751: underlyingBalance - underlyingToWithdraw; //because of the condition line 745, the underflow check is unnecessary
BkdEthCvx.sol:83: uint256 requiredUnderlyingAmount = amount - underlyingBalance; //because of the condition line 76, the underflow check is unnecessary
BkdTriHopCvx.sol:181: uint256 requiredUnderlyingAmount = amount - underlyingBalance; //because of the condition line 174, the underflow check is unnecessary
CvxMintAmount.sol:24: uint256 remaining = _CLIFF_COUNT - currentCliff; //because of the condition line 21, the underflow check is unnecessary
Vault.sol:24: uint256 remaining = _CLIFF_COUNT - currentCliff; //because of the condition line 21, the underflow check is unnecessary
Vault.sol:125: uint256 requiredWithdrawal = amount - availableUnderlying_; //because of the condition line 122, the underflow check is unnecessary
Vault.sol:130: uint256 newTarget = (allocated - requiredWithdrawal) //because of the condition line 127, the underflow check is unnecessary
Vault.sol:141: uint256 totalUnderlyingAfterWithdraw = totalUnderlying - amount; //because of the condition line 122, the underflow check is unnecessary
Vault.sol:440: waitingForRemovalAllocated = _waitingForRemovalAllocated - withdrawn; //because of the condition line 437, the underflow check is unnecessary
Vault.sol:444: uint256 profit = withdrawn - allocated; //because of the condition line 443, the underflow check is unnecessary
Vault.sol:452: allocated -= withdrawn; //because of the condition line 443, the underflow check is unnecessary
Vault.sol:591: uint256 profit = allocatedUnderlying - amountAllocated; //because of the condition line 589, the underflow check is unnecessary
Vault.sol:595: profit -= currentDebt; //because of the condition line 593, the underflow check is unnecessary
Vault.sol:600: currentDebt -= profit; //because of the condition line 593, the underflow check is unnecessary
Vault.sol:605: uint256 loss = amountAllocated - allocatedUnderlying; //because of the condition line 603, the underflow check is unnecessary
Vault.sol:784: uint256 withdrawAmount = allocatedUnderlying - target; //because of the condition line 782, the underflow check is unnecessary
Vault.sol:790: uint256 depositAmount = target - allocatedUnderlying; //because of the condition line 788, the underflow check is unnecessary
StakerVault.sol:164: uint256 srcTokensNew = srcTokens - amount; //because of the condition line 153, the underflow check is unnecessary
Place the arithmetic operations in an unchecked block
chase-manning (Backd) resolved and commented:
I consider this report to be of particularly high quality.
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.