Platform: Code4rena
Start Date: 25/07/2023
Pot Size: $80,100 USDC
Total HM: 18
Participants: 7
Period: 10 days
Judge: cccz
Total Solo HM: 15
Id: 268
League: ETH
Rank: 1/7
Findings: 3
Award: $0.00
🌟 Selected for report: 4
🚀 Solo Findings: 3
🌟 Selected for report: RaymondFam
Data not available
When unwrapping the wComet to its rebasing comet, users with an equivalent amount of wComet invoking CusdcV3Wrapper._withdraw at around the same time could end up having different percentage gains because comet is not linearly rebasing.
Moreover, the rate-determining getUpdatedSupplyIndicies() is an internal view function inaccessible to the users unless they take the trouble creating a contract to inherit CusdcV3Wrapper.sol. So most users making partial withdrawals will have no clue whether or not this is the best time to unwrap. This is because the public view function underlyingBalanceOf is only directly informational when amount has been entered as type(uint256).max.
function _withdraw( address operator, address src, address dst, uint256 amount ) internal { if (!hasPermission(src, operator)) revert Unauthorized(); // {Comet} uint256 srcBalUnderlying = underlyingBalanceOf(src); if (srcBalUnderlying < amount) amount = srcBalUnderlying; if (amount == 0) revert BadAmount(); underlyingComet.accrueAccount(address(this)); underlyingComet.accrueAccount(src); uint256 srcBalPre = balanceOf(src); CometInterface.UserBasic memory wrappedBasic = underlyingComet.userBasic(address(this)); int104 wrapperPrePrinc = wrappedBasic.principal; // conservative rounding in favor of the wrapper IERC20(address(underlyingComet)).safeTransfer(dst, (amount / 10) * 10); wrappedBasic = underlyingComet.userBasic(address(this)); int104 wrapperPostPrinc = wrappedBasic.principal; // safe to cast because principal can't go negative, wrapper is not borrowing uint256 burnAmt = uint256(uint104(wrapperPrePrinc - wrapperPostPrinc)); // occasionally comet will withdraw 1-10 wei more than we asked for. // this is ok because 9 times out of 10 we are rounding in favor of the wrapper. // safe because we have already capped the comet withdraw amount to src underlying bal. // untested: // difficult to trigger, depends on comet rules regarding rounding if (srcBalPre <= burnAmt) burnAmt = srcBalPre; accrueAccountRewards(src); _burn(src, safe104(burnAmt)); }
As can be seen in the code block of function _withdraw above, underlyingBalanceOf(src) is first invoked.
function underlyingBalanceOf(address account) public view returns (uint256) { uint256 balance = balanceOf(account); if (balance == 0) { return 0; } return convertStaticToDynamic(safe104(balance)); }
Next, function convertStaticToDynamic is invoked.
function convertStaticToDynamic(uint104 amount) public view returns (uint256) { (uint64 baseSupplyIndex, ) = getUpdatedSupplyIndicies(); return presentValueSupply(baseSupplyIndex, amount); }
And next, function getUpdatedSupplyIndicies is invoked. As can be seen in its code logic, the returned value of baseSupplyIndex_ is determined by the changing supplyRate.
function getUpdatedSupplyIndicies() internal view returns (uint64, uint64) { TotalsBasic memory totals = underlyingComet.totalsBasic(); uint40 timeDelta = uint40(block.timestamp) - totals.lastAccrualTime; uint64 baseSupplyIndex_ = totals.baseSupplyIndex; uint64 trackingSupplyIndex_ = totals.trackingSupplyIndex; if (timeDelta > 0) { uint256 baseTrackingSupplySpeed = underlyingComet.baseTrackingSupplySpeed(); uint256 utilization = underlyingComet.getUtilization(); uint256 supplyRate = underlyingComet.getSupplyRate(utilization); baseSupplyIndex_ += safe64(mulFactor(baseSupplyIndex_, supplyRate * timeDelta)); trackingSupplyIndex_ += safe64( divBaseWei(baseTrackingSupplySpeed * timeDelta, totals.totalSupplyBase) ); } return (baseSupplyIndex_, trackingSupplyIndex_); }
The returned value of baseSupplyIndex is then inputted into function principalValueSupply where the lower the value of baseSupplyIndex, the higher the principalValueSupply or simply put, the lesser the burn amount.
function principalValueSupply(uint64 baseSupplyIndex_, uint256 presentValue_) internal pure returns (uint104) { return safe104((presentValue_ * BASE_INDEX_SCALE) / baseSupplyIndex_); }
Manual
Consider implementing slippage protection on CusdcV3Wrapper._withdraw so that users could opt for the minimum amount of comet to receive or the maximum amount of wComet to burn.
Timing
#0 - c4-judge
2023-08-05T14:52:30Z
thereksfour marked the issue as primary issue
#1 - c4-sponsor
2023-08-08T20:32:48Z
tbrent marked the issue as sponsor acknowledged
#2 - pmckelvy1
2023-08-22T15:29:36Z
users can always use convertStaticToDynamic and convertDynamicToStatic to get the exchange rates as they both use getUpdatedSupplyIndicies(). the issue being flagged here (rebase rate is dynamic) is inherent to the comet itself (and pretty much any rebasing token for that matter), and not something the wrapper needs to be concerned about.
#3 - c4-judge
2023-09-05T06:23:15Z
thereksfour marked the issue as satisfactory
#4 - c4-judge
2023-09-05T06:23:20Z
thereksfour marked the issue as selected for report
🌟 Selected for report: RaymondFam
Data not available
The Aave plugin is associated with an ever-increasing exchange rate. The earlier a user wraps the AToken, the more Static Atoken will be minted and understandably no slippage protection is needed.
However, since the rate is not linearly increasing, withdrawing the Static Atoken (following RToken redemption) at the wrong time could mean a difference in terms of the amount of AToken redeemed. The rate could be in a transient mode of non-increasing or barely increasing and then a significant surge. Users with an equivalent amount of Static AToken making such calls at around the same time could end up having different percentage gains.
Although the user could always deposit and wrap the AToken again, it's not going to help if the wrapping were to encounter a sudden surge (bad timing again) thereby thwarting the intended purpose.
uint256 userBalance = balanceOf(owner); uint256 amountToWithdraw; uint256 amountToBurn; uint256 currentRate = rate(); if (staticAmount > 0) { amountToBurn = (staticAmount > userBalance) ? userBalance : staticAmount; amountToWithdraw = _staticToDynamicAmount(amountToBurn, currentRate); } else { uint256 dynamicUserBalance = _staticToDynamicAmount(userBalance, currentRate); amountToWithdraw = (dynamicAmount > dynamicUserBalance) ? dynamicUserBalance : dynamicAmount; amountToBurn = _dynamicToStaticAmount(amountToWithdraw, currentRate); } _burn(owner, amountToBurn);
As can be seen in the code block of function _withdraw above, choosing staticAmount > 0 will have a lesser amount of AToken to withdraw when the currenRate is stagnant.
function _staticToDynamicAmount(uint256 amount, uint256 rate_) internal pure returns (uint256) { return amount.rayMul(rate_); }
Similarly, choosing dynamicAmount > 0 will have a higher than expected amount of Static Atoken to burn.
function _dynamicToStaticAmount(uint256 amount, uint256 rate_) internal pure returns (uint256) { return amount.rayDiv(rate_); }
Manual
Consider implementing slippage protection on StaticATokenLM._withdraw so that users could opt for the minimum amount of AToken to receive or the maximum amount of Static Atoken to burn.
Timing
#0 - c4-judge
2023-08-05T15:10:50Z
thereksfour marked the issue as primary issue
#1 - c4-sponsor
2023-08-08T20:33:05Z
tbrent marked the issue as sponsor acknowledged
#2 - julianmrodri
2023-08-28T14:24:55Z
HI, as mentioned before we acknowledge the existence of this issue.
But on the other hand we will not implement fixes or changes. We believe it is the responsibility of the user to decide when to wrap/unwrap these tokens and these interactions are in general outside of the protocol behavior.
In addition to this the rate is accesible for the user to make that decision, and we don't expect these rates to increase abruptly for this wrapper, so in reality we might be adding a feature that will probably not be used in practice.
It is important to remark this is something that exists in any wrapper for rebasing tokens we use, wether it is our own, or developed by other protocol teams. And in generally we don't see implemented in those wrappers slippage protection or a feature like the one suggested here.
#3 - c4-judge
2023-09-05T06:22:56Z
thereksfour marked the issue as satisfactory
#4 - c4-judge
2023-09-05T06:23:00Z
thereksfour marked the issue as selected for report
🌟 Selected for report: RaymondFam
Data not available
This issue could lead to a permanent loss of rewards for the transferer of the token. During the token transfer process, the _beforeTokenTransfer function updates rewards for both the sender and the receiver. However, due to the specific call order and the behavior of the _updateUser function and the _getPendingRewards function, some rewards may not be accurately accounted for.
The crux of the problem lies in the fact that the _getPendingRewards function, when called with the fresh parameter set to false, may not account for all the latest rewards from the INCENTIVES_CONTROLLER. As a result, the _updateUserSnapshotRewardsPerToken function, which relies on the output of the _getPendingRewards function, could end up missing out on some rewards. This could be detrimental to the token sender, especially the Backing Manager whenever RToken is redeemed. Apparently, most users having wrapped their AToken, would automatically use it to issue RToken as part of the basket range of backing collaterals and be minimally impacted. But it would have the Reserve Protocol collectively affected depending on the frequency and volume of RToken redemption and the time elapsed since _updateRewards() or _collectAndUpdateRewards() was last called. The same impact shall also apply when making token transfers to successful auction bidders.
As denoted in the function NatSpec below, function _beforeTokenTransfer updates rewards for senders and receivers in a transfer.
/** * @notice Updates rewards for senders and receiver in a transfer (not updating rewards for address(0)) * @param from The address of the sender of tokens * @param to The address of the receiver of tokens */ function _beforeTokenTransfer( address from, address to, uint256 ) internal override { if (address(INCENTIVES_CONTROLLER) == address(0)) { return; } if (from != address(0)) { _updateUser(from); } if (to != address(0)) { _updateUser(to); } }
When function _updateUser is respectively invoked, _getPendingRewards(user, balance, false) is called.
/** * @notice Adding the pending rewards to the unclaimed for specific user and updating user index * @param user The address of the user to update */ function _updateUser(address user) internal { uint256 balance = balanceOf(user); if (balance > 0) { uint256 pending = _getPendingRewards(user, balance, false); _unclaimedRewards[user] = _unclaimedRewards[user].add(pending); } _updateUserSnapshotRewardsPerToken(user); }
However, because the third parameter has been entered false, the third if block of function _getPendingRewards is skipped.
/** * @notice Compute the pending in RAY (rounded down). Pending is the amount to add (not yet unclaimed) rewards in RAY (rounded down). * @param user The user to compute for * @param balance The balance of the user * @param fresh Flag to account for rewards not claimed by contract yet * @return The amound of pending rewards in RAY */ function _getPendingRewards( address user, uint256 balance, bool fresh ) internal view returns (uint256) { if (address(INCENTIVES_CONTROLLER) == address(0)) { return 0; } if (balance == 0) { return 0; } uint256 rayBalance = balance.wadToRay(); uint256 supply = totalSupply(); uint256 accRewardsPerToken = _accRewardsPerToken; if (supply != 0 && fresh) { address[] memory assets = new address[](1); assets[0] = address(ATOKEN); uint256 freshReward = INCENTIVES_CONTROLLER.getRewardsBalance(assets, address(this)); uint256 lifetimeRewards = _lifetimeRewardsClaimed.add(freshReward); uint256 rewardsAccrued = lifetimeRewards.sub(_lifetimeRewards).wadToRay(); accRewardsPerToken = accRewardsPerToken.add( (rewardsAccrued).rayDivNoRounding(supply.wadToRay()) ); } return rayBalance.rayMulNoRounding(accRewardsPerToken.sub(_userSnapshotRewardsPerToken[user])); }
Hence, accRewardsPerToken may not be updated with the latest rewards from INCENTIVES_CONTROLLER. Consequently, _updateUserSnapshotRewardsPerToken(user) will miss out on claiming some rewards and is a loss to the StaticAtoken transferrer.
/** * @notice Update the rewardDebt for a user with balance as his balance * @param user The user to update */ function _updateUserSnapshotRewardsPerToken(address user) internal { _userSnapshotRewardsPerToken[user] = _accRewardsPerToken; }
Ironically, this works out as a zero-sum game where the loss of the transferrer is a gain to the transferee. But most assuredly, the Backing Manager is going to end up incurring more losses than gains in this regard.
Manual
Consider introducing an additional call to update the state of rewards before any token transfer occurs. Specifically, within the _beforeTokenTransfer function, invoking _updateRewards like it has been implemented in StaticATokenLM._deposit and StaticATokenLM._withdraw before updating the user balances would have the issues resolved.
This method would not force an immediate claim of rewards, but rather ensure the internal accounting of rewards is up-to-date before the transfer. By doing so, the state of rewards for each user would be accurate and ensure no loss or premature gain of rewards during token transfers.
Token-Transfer
#0 - c4-judge
2023-08-05T08:31:12Z
thereksfour marked the issue as primary issue
#1 - c4-judge
2023-08-06T16:10:26Z
thereksfour changed the severity to 2 (Med Risk)
#2 - tbrent
2023-08-08T20:41:12Z
#3 - julianmrodri
2023-08-28T14:51:31Z
We will mark this issue as Sponsor Acknowledged. It is true the situation described by the warden and that's the behavior we observe. However we will not be implementing any change in the code (besides adding some comments) for the following reasons:
transfer operation. It is important to remark that any deposit or withdraw done to the contract plus any call to collectRewards.., and any claim of rewards from the Reserve protocol, would setup the correct balances. So while it is true that transfers may in some cases not transfer rewards we expect this to only be slightly off.To clarify this issue for users we will add a comment to the wrapper contract StaticATokenLM to clarify the situation with how rewards are handled on transfer.
#4 - c4-sponsor
2023-08-28T16:16:04Z
tbrent marked the issue as sponsor acknowledged
#5 - c4-judge
2023-09-05T06:22:31Z
thereksfour marked the issue as satisfactory
#6 - c4-judge
2023-09-05T06:22:35Z
thereksfour marked the issue as selected for report
🌟 Selected for report: auditor0517
Also found by: 0xA5DF, RaymondFam, bin2chen, carlitox477, ronnyx2017
Data not available
StaticATokenLM._withdrawThe following require statement might need some refactoring to better accomplish its intended purpose. First off, it does not capture whether or not both staticAmount and dynamicAmount are zeroes at the same time.
require( staticAmount == 0 || dynamicAmount == 0, StaticATokenErrors.ONLY_ONE_AMOUNT_FORMAT_ALLOWED );
Secondly, staticAmount and dynamicAmount have separately been hardcoded to 0 in the following two calling functions.
return _withdraw(msg.sender, recipient, amount, 0, toUnderlying);
return _withdraw(msg.sender, recipient, 0, amount, toUnderlying);
Here's a suggested fix:
require( (staticAmount == 0 && dynamicAmount != 0) || (dynamicAmount == 0 && staticAmount != 0), StaticATokenErrors.ONLY_ONE_AMOUNT_FORMAT_ALLOWED );
The plugin rewards are claimable via the following methods:
accRewardsPerToken multiplied by the pro-rate static share as adopted in StaticATokenLMaccrued - claimed as adopted in CusdcV3Wrapper.solbalanceAfterClaimingRewards - _previousBalance featured by RewardableERC20.sol and inherited by other plugins like MorphoTokenisedDeposit.solWhile no loss of funds is involved here, all residual balances due to accidentally sent in, failure of transfers, etc will be forever stuck in the contract.
- uint48 delayUntilDefault; // {s} The number of seconds an oracle can mulfunction + uint48 delayUntilDefault; // {s} The number of seconds an oracle can malfunction
- // In this case, the asset may recover, reachiving _whenDefault == NEVER. + // In this case, the asset may recover, reachieving _whenDefault == NEVER.
- * @notice Updates rewards for senders and receiver in a transfer (not updating rewards for address(0)) + * @notice Updates rewards for senders and receivers in a transfer (not updating rewards for address(0))
- * @notice Compute the pending in RAY (rounded down). Pending is the amount to add (not yet unclaimed) rewards in RAY (rounded down). + * @notice Compute the pending in RAY (rounded down). Pending is the amount to add (not yet claimed) rewards in RAY (rounded down).
- /// Takes `amount` fo cUSDCv3 from `src` and deposits to `dst` account in the wrapper. + /// Takes `amount` of cUSDCv3 from `src` and deposits to `dst` account in the wrapper.
- // an immutable array so we do not have store the token feeds in the blockchain. This is + // an immutable array so we do not have to store the token feeds in the blockchain. This is
- // I know this lots extremely verbose and quite silly, but it actually makes sense: + // I know this is extremely verbose and quite silly, but it actually makes sense:
FiatCollateral.alreadyDefaulted turns obsolete when block.timestamp > type(uint48).maxAlthough this will take a very long time to happen, the following function will default to true, i.e. DISABLED when block.timestamp exceeds type(uint48).max considering _whenDefault would at most be assigned NEVER, which is type(uint48).max.
function alreadyDefaulted() internal view returns (bool) { return _whenDefault <= block.timestamp; }
@return NatSpec of EURFiatCollateral.tryPriceThe unit of pegPrice in EURFiatCollateral.tryPrice is target/ref but it is stated otherwise in the function NatSpec.
- /// @return pegPrice {UoA/ref} + /// @return pegPrice {target/ref}
else clause of Asset.lotPrice<= and >= have respectively been used in the if and else if clauses, making the = sign of the inequalities in the commented else clause erroneous.
uint48 delta = uint48(block.timestamp) - lastSave; // {s} if (delta <= oracleTimeout) { lotLow = savedLowPrice; lotHigh = savedHighPrice; } else if (delta >= oracleTimeout + priceTimeout) { return (0, 0); // no price after full timeout } else { - // oracleTimeout <= delta <= oracleTimeout + priceTimeout + // oracleTimeout < delta < oracleTimeout + priceTimeout // {1} = {s} / {s} uint192 lotMultiplier = divuu(oracleTimeout + priceTimeout - delta, priceTimeout); // {UoA/tok} = {UoA/tok} * {1} lotLow = savedLowPrice.mul(lotMultiplier); lotHigh = savedHighPrice.mul(lotMultiplier); }
The comment below is missing out the units pertaining to the division by one.
- // {qRewards} = {qRewards/share} * {qShare} - // {qRewards} = {qRewards/share} * {qShare} / {qShare/share} _accumuatedRewards += (delta * shares) / one;
State variable names should be Camel-cased instead of fully capitalized (typically used on constants and immutables).
Here are some of the instances entailed:
ILendingPool public override LENDING_POOL; IAaveIncentivesController public override INCENTIVES_CONTROLLER; IERC20 public override ATOKEN; IERC20 public override ASSET; IERC20 public override REWARD_TOKEN;
For cleaner Solidity code in conjunction with the rule of modularity and modular programming, use named imports with curly braces instead of adopting the global import approach.
For example, the import instances below could be refactored conforming to the suggested standards as follows:
- import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; + import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; - import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; + import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; - import "@openzeppelin/contracts/token/ERC20/ERC20.sol"; + import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; - import "./RewardableERC20.sol"; + import {RewardableERC20} from "./RewardableERC20.sol";
It has lately been suggested that using enums instead of numbers for designated array indices makes the code logic more descriptive and readable.
Here are some of the instances entailed:
132: token0 = tokens[0]; 133: token1 = tokens[1]; 144: bool more = config.feeds[0].length > 0; 147: _t0feed0 = more ? config.feeds[0][0] : AggregatorV3Interface(address(0)); 148: _t0timeout0 = more && config.oracleTimeouts[0].length > 0 ? config.oracleTimeouts[0][0] : 0; 149: _t0error0 = more && config.oracleErrors[0].length > 0 ? config.oracleErrors[0][0] : 0;
Starting with Solidity version [0.8.8](https://docs.soliditylang.org/en/v0.8.20/contracts.html#function-overriding], using the override keyword when the function solely overrides an interface function, and the function doesn't exist in multiple base contracts, is unnecessary.
Here are some of the instances entailed:
https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/NonFiatCollateral.sol#L38-L42 https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/SelfReferentialCollateral.sol#L26-L30
function tryPrice() external view override returns (
49: function _underlyingRefPerTok() internal view override returns (uint192) { 56: function claimRewards() external virtual override(Asset, IRewardable) {
Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables, and more. Please visit the following link for further details:
https://docs.soliditylang.org/en/v0.8.16/natspec-format.html
It's generally incomplete throughout all codebases. Consider fully equipping all contracts with a complete set of NatSpec to better facilitate users/developers interacting with the protocol's smart contracts.
According to Solidity's Style Guide below:
https://docs.soliditylang.org/en/v0.8.17/style-guide.html
In order to help readers identify which functions they can call, and find the constructor and fallback definitions more easily, functions should be grouped according to their visibility and ordered in the following manner:
constructor, receive function (if exists), fallback function (if exists), external, public, internal, private
And, within a grouping, place the view and pure functions last.
Additionally, inside each contract, library or interface, use the following order:
type declarations, state variables, events, modifiers, functions
Consider adhering to the above guidelines for all contract instances entailed.
Passing zero as a function argument can sometimes result in a security issue (e.g. passing zero as the slippage parameter, asset amount, etc). Consider using a constant variable with a descriptive name, so it's clear that the argument is intentionally being used, and for the right reasons.
Here are some of the instances entailed:
return _withdraw(msg.sender, recipient, amount, 0, toUnderlying);
return _withdraw(msg.sender, recipient, 0, amount, toUnderlying);
The interfaces below should be defined in separate files, so that it's easier for future projects to import them, and to avoid duplication later on if they need to be used elsewhere in the project.
Here is one specific instance entailed that has been embedded in ATokenFiatCollateral.sol:
interface IStaticAToken is IERC20Metadata {
#0 - c4-judge
2023-08-06T10:16:12Z
thereksfour marked the issue as grade-a
🌟 Selected for report: RaymondFam
Also found by: carlitox477
Data not available
The use of constant keccak variables results in extra hashing whenever the variable is used, increasing gas costs relative to just storing the output hash. Changing to immutable will only perform hashing on contract deployment which will save gas.
Here are some of the instances entailed.
bytes public constant EIP712_REVISION = bytes("1"); bytes32 internal constant EIP712_DOMAIN = keccak256( "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)" ); bytes32 public constant PERMIT_TYPEHASH = keccak256( "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)" ); bytes32 public constant METADEPOSIT_TYPEHASH = keccak256( "Deposit(address depositor,address recipient,uint256 value,uint16 referralCode,bool fromUnderlying,uint256 nonce,uint256 deadline)" ); bytes32 public constant METAWITHDRAWAL_TYPEHASH = keccak256( "Withdraw(address owner,address recipient,uint256 staticAmount,uint256 dynamicAmount,bool toUnderlying,uint256 nonce,uint256 deadline)" );
The following else block in the function logic of refresh() is never reachable considering tryPrice() does not have (0, FIX_MAX) catered for. Any upriced data would have been sent to the catch block. Other than Asset.sol, similarly wasted logic is also exhibited in AppreciatingFiatCollateral.refresh, FiatCollateral.refresh, CTokenV3Collateral.refresh, CurveStableCollateral.refresh, StargatePoolFiatCollateral.refresh.
/// Should not revert /// Refresh saved prices function refresh() public virtual override { try this.tryPrice() returns (uint192 low, uint192 high, uint192) { // {UoA/tok}, {UoA/tok} // (0, 0) is a valid price; (0, FIX_MAX) is unpriced // Save prices if priced if (high < FIX_MAX) { savedLowPrice = low; savedHighPrice = high; lastSave = uint48(block.timestamp); } else { // must be unpriced assert(low == 0); } } catch (bytes memory errData) { // see: docs/solidity-style.md#Catching-Empty-Data if (errData.length == 0) revert(); // solhint-disable-line reason-string } }
return in the try clauseThis try clause has already returned low and high and is returning the same things again in its nested logic, which is inexpedient and unnecessary. Similar behavior is also found in Asset.price.
function price() public view virtual returns (uint192, uint192) { try this.tryPrice() returns (uint192 low, uint192 high) { - return (low, high); } catch (bytes memory errData) { // see: docs/solidity-style.md#Catching-Empty-Data if (errData.length == 0) revert(); // solhint-disable-line reason-string return (0, FIX_MAX); } }
RTokenAsset.solIRToken.sol has already been imported by IMain.sol, making the former an unneeded import.
import "../../interfaces/IMain.sol"; - import "../../interfaces/IRToken.sol";
In StaticATokenLM._updateRewards, the state variable _lifetimeRewardsClaimed is doubly used in the following code logic when rewardsAccrued could simply/equally be assigned freshRewards.wadToRay(). This extra gas incurring behaviour is also exhibited in StaticATokenLM._collectAndUpdateRewards, and StaticATokenLM._getPendingRewards.
uint256 freshRewards = INCENTIVES_CONTROLLER.getRewardsBalance(assets, address(this)); uint256 lifetimeRewards = _lifetimeRewardsClaimed.add(freshRewards); - uint256 rewardsAccrued = lifetimeRewards.sub(_lifetimeRewards).wadToRay(); + uint256 rewardsAccrued = freshRewards.wadToRay();
The if block in the constructor of PoolTokens.sol entails identical if block checks that may be moved outside the loop to save gas.
IERC20Metadata[] memory tokens = new IERC20Metadata[](nTokens); + if (config.poolType == CurvePoolType.Plain) revert("invalid poolType"); for (uint8 i = 0; i < nTokens; ++i) { - if (config.poolType == CurvePoolType.Plain) { tokens[i] = IERC20Metadata(curvePool.coins(i)); - } else { - revert("invalid poolType"); - } }
In the constructor of PoolTokens.sol, the second condition of the following require statement mandates that at least one feed is associated with each token.
require( config.feeds.length == config.nTokens && minFeedsLength(config.feeds) > 0, "each token needs at least 1 price feed" );
Additionally, the following code lines signify that a minimum of 2 tokens will be associated with the Curve base pool. Otherwise, if nTokens is less than 2 or 1, assigning token0 and/or token1 will revert due to accessing out-of-bound array elements.
token0 = tokens[0]; token1 = tokens[1]; token2 = (nTokens > 2) ? tokens[2] : IERC20Metadata(address(0)); token3 = (nTokens > 3) ? tokens[3] : IERC20Metadata(address(0));
Under this context, the following ternary logic along with the use of boolean more is therefore deemed unnecessary.
// token0 - bool more = config.feeds[0].length > 0; // untestable: // more will always be true based on previous feeds validations - _t0feed0 = more ? config.feeds[0][0] : AggregatorV3Interface(address(0)); + _t0feed0 = config.feeds[0][0]; _t0timeout0 = more && config.oracleTimeouts[0].length > 0 ? config.oracleTimeouts[0][0] : 0; _t0error0 = more && config.oracleErrors[0].length > 0 ? config.oracleErrors[0][0] : 0; - if (more) { require(address(_t0feed0) != address(0), "t0feed0 empty"); require(_t0timeout0 > 0, "t0timeout0 zero"); require(_t0error0 < FIX_ONE, "t0error0 too large"); - }
// token1 // untestable: // more will always be true based on previous feeds validations - more = config.feeds[1].length > 0; - _t1feed0 = more ? config.feeds[1][0] : AggregatorV3Interface(address(0)); + _t1feed0 = config.feeds[1][0]; _t1timeout0 = more && config.oracleTimeouts[1].length > 0 ? config.oracleTimeouts[1][0] : 0; _t1error0 = more && config.oracleErrors[1].length > 0 ? config.oracleErrors[1][0] : 0; - if (more) { require(address(_t1feed0) != address(0), "t1feed0 empty"); require(_t1timeout0 > 0, "t1timeout0 zero"); require(_t1error0 < FIX_ONE, "t1error0 too large"); - }
Similarly, the following second conditional checks are also not needed.
// token2 - more = config.feeds.length > 2 && config.feeds[2].length > 0; + more = config.feeds.length > 2;
// token3 - more = config.feeds.length > 3 && config.feeds[3].length > 0; + more = config.feeds.length > 3;
You can have further advantages in terms of gas cost by simply using named return values as temporary local variables.
For instance, the code block below may be refactored as follows:
- function underlyingBalanceOf(address account) public view returns (uint256) { + function underlyingBalanceOf(address account) public view returns (uint256 _balance) { uint256 balance = balanceOf(account); if (balance == 0) { return 0; } - return convertStaticToDynamic(safe104(balance)); + _balance = convertStaticToDynamic(safe104(balance)); }
+= and -= generally cost 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop.
For instance, the += instance below may be refactored as follows:
- baseSupplyIndex_ += safe64(mulFactor(baseSupplyIndex_, supplyRate * timeDelta)); + baseSupplyIndex_ = baseSupplyIndex_ + safe64(mulFactor(baseSupplyIndex_, supplyRate * timeDelta));
The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:
Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.
module.exports = { solidity: { version: "0.8.19", settings: { optimizer: { enabled: true, runs: 1000, }, }, }, };
Please visit the following site for further information:
https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler
Here's one example of instance on opcode comparison that delineates the gas saving mechanism:
for !=0 before optimization PUSH1 0x00 DUP2 EQ ISZERO PUSH1 [cont offset] JUMPI after optimization DUP1 PUSH1 [revert offset] JUMPI
Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past. A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.
Payable functions cost less gas to execute, since the compiler does not have to add extra checks to ensure that a payment wasn't provided. A constructor can safely be marked as payable, since only the deployer would be able to pass funds, and the project itself would not pass any funds.
Here are some of the instances entailed:
constructor(CollateralConfig memory config, uint192 revenueHiding)
constructor( CollateralConfig memory config, AggregatorV3Interface targetUnitChainlinkFeed_, uint48 targetUnitOracleTimeout_ ) FiatCollateral(config) {
constructor(CollateralConfig memory config) Asset( config.priceTimeout, config.chainlinkFeed, config.oracleError, config.erc20, config.maxTradeVolume, config.oracleTimeout )
If the arguments to the encode call can fit into the scratch space (two words or fewer), then it's more efficient to use assembly to generate the hash (80 gas): keccak256(abi.encodePacked(x, y)) -> assembly {mstore(0x00, a); mstore(0x20, b); let hash := keccak256(0x00, 0x40); }
Here are some of the instances entailed:
keccak256(bytes(name())), keccak256(EIP712_REVISION),
Please visit the following link for substantiated details:
And, here are some of the instances entailed:
pragma solidity 0.6.12;
pragma solidity 0.6.12;
External calls have an overhead of 100 gas, which can be avoided by not referencing the function using this. Contracts are allowed to override their parents' functions and change the visibility from external to public, so make this change if it's required in order to call the function internally.
Here are some of the instances entailed:
try this.tryPrice() returns (uint192 low, uint192 high, uint192 pegPrice) {
89: try this.tryPrice() returns (uint192 low, uint192 high, uint192) { 113: try this.tryPrice() returns (uint192 low, uint192 high, uint192) { 129: try this.tryPrice() returns (uint192 low, uint192 high, uint192) {
#0 - c4-judge
2023-08-05T15:36:27Z
thereksfour marked the issue as grade-a
#1 - c4-judge
2023-08-05T15:45:38Z
thereksfour marked the issue as selected for report