Platform: Code4rena
Start Date: 06/01/2023
Pot Size: $210,500 USDC
Total HM: 27
Participants: 73
Period: 14 days
Judge: 0xean
Total Solo HM: 18
Id: 203
League: ETH
Rank: 26/73
Findings: 1
Award: $1,004.64
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: CodingNameKiki
Also found by: 0xA5DF, 0xAgro, 0xNazgul, 0xSmartContract, Aymen0909, BRONZEDISC, Bnke0x0, Breeje, Cyfrin, GalloDaSballo, HollaDieWaldfee, IceBear, IllIllI, MyFDsYours, RaymondFam, Ruhum, SaharDevep, Sathish9098, Soosh, Udsen, __141345__, brgltd, btk, carlitox477, chaduke, chrisdior4, cryptonue, delfin454000, descharre, hihen, joestakey, ladboy233, lukris02, luxartvinsec, peanuts, pedr02b2, rotcivegaf, shark, tnevler, yongskiws
1004.6363 USDC - $1,004.64
Some ERC20s revert on transfer of zero amount. Whenever making more than one transfer in the same function, it's recommended to check for zero amount.
Otherwise, if a token with such behavior in used for one of the transfers, the entire transaction will fail including the transfer of tokens with standard behavior.
Consider making the following change in RToken.sol
diff --git a/RToken.sol.orig b/RToken.sol --- a/RToken.sol.orig +++ b/RToken.sol @@ -791,10 +791,12 @@ contract RTokenP1 is ComponentP1, ERC20PermitUpgradeable, IRToken { _mint(account, amtRToken); for (uint256 i = 0; i < tokensLen; ++i) { - IERC20Upgradeable(queue.tokens[i]).safeTransfer( - address(backingManager), - amtDeposits[i] - ); + if (amtDeposits[i] != 0) { + IERC20Upgradeable(queue.tokens[i]).safeTransfer( + address(backingManager), + amtDeposits[i] + ); + } } }
Distributor.distribute()The distribute function will send funds (calculated from tokensPerShare * numberOfShares) from the user controlled input from. This can be called by a griefer at unwanted times.
Consider adding access control into this function so that it can only be called by trusted addresses.
The project downcast integers on multiple locations throughout the codebase. In many places, there's a comment explaining why downcasting without a check is safe, e.g.
However, some operations are downcasting and don't contain any comment contextualizing why it's safe, e.g.
Consider adding a commend on all direct downcasting operations explaining why it's safe. For any place considered not safe, consider using a safe cast function that reverts if the number being downcasted causes an overflow, e.g. OpenZeppelin SafeCast, similarly to how it's implemented on StRSRVotes.
This way, the project can: use safe cast wherever necessary, and have a comment for any place that uses direct downcasting without a safe wrapper.
The majority of contracts are using v0.8.9. Consider using the latest version of solidity to ensure the compiler contains the latest security fixes and the latest features, e.g. v.0.8.10 contains some new optimizations with regards to external function calls
Consider checking against address(0) during address initializations.
If a variable gets configured with address zero, failure to immediately reset the value can result in unexpected behavior for the project.
E.g. the following checks can be added in Distributor.sol:
diff --git a/Distributor.sol.orig b/Distributor.sol --- a/Distributor.sol.orig +++ b/Distributor.sol @@ -42,10 +42,15 @@ contract DistributorP1 is ComponentP1, IDistributor { __Component_init(main_); rsr = main_.rsr(); + + require(address(main_.rToken()) != address(0), "invalid rToken address"); + require(address(main_.furnace()) != address(0), "invalid furnace address"); + require(address(main_.stRSR()) != address(0), "invalid stRSR address"); + rToken = IERC20(address(main_.rToken())); furnace = address(main_.furnace()); stRSR = address(main_.stRSR());
When possible, prevent state changes when the system is in pause mode.
E.g. BackingManager.sol won't execute the following functions if the contract is paused: grantRTokenAllowance(), manageTokens() and manageTokensSortedOrder(). However, state variables like tradingDelay and backingBuffer can be modified. Consider adding the modifier notPausedOrFrozen into setTradingDelay() and setBackingBuffer().
Consider adding a check for the price returned by chainlink to ensure it's positive. The impact of not using this type of check is letting an incorrect price value being processed.
Add the following change on OracleLib.sol:
diff --git a/OracleLib.sol.orig b/OracleLib.sol --- a/OracleLib.sol.orig +++ b/OracleLib.sol @@ -26,6 +26,8 @@ library OracleLib { (uint80 roundId, int256 p, , uint256 updateTime, uint80 answeredInRound) = chainlinkFeed .latestRoundData(); if (updateTime == 0 || answeredInRound < roundId) { revert StalePrice(); } // Downcast is safe: uint256(-) reverts on underflow; block.timestamp assumed < 2^48 uint48 secondsSince = uint48(block.timestamp - updateTime); if (secondsSince > timeout) revert StalePrice(); + if (p <= 0) revert CustomError(); + // {UoA/tok} return shiftl_toFix(uint256(p), -int8(chainlinkFeed.decimals())); }
oracleError to be modifiedCurrently Asset.oracleError is immutable. Consider adding a setter function to allow this value to be modified after deployment. Otherwise, there might be slippage issues locking the system during market turbulance, preveting users from interacting with the asset.
The loop in BasketHandler._switchBasket() can run out of gas if there are too many ERC20 items into one config state variable. Impact can be a DOS condition when calling refreshBasket().
Consider adding new functions to allow switching a basket through multiple calls, by saving intermediare values and passing a range of indexes to be iterated for config, e.g. adding a slice functionality.
BasketNeededChanged after the external call battery.discarge() in RToken.redeem()Consider emitting the event after the external call. Since the function doesn't contain a nonReentrant modifier, reentrant calls could spam the event and disturbe frontends or monitoring tools.
Check if the new value is different from the current value. This can help prevent emitting unnecessary events.
abi.encode is generally preferred over abi.encodePacked to prevent hash collisions. If there's a compelling reason to use abi.encodePacked, consider adding a comment.
Repeated validation statements can be converted into a function modifier to improve code reusability.
__gap can be declared into a separate contract and reused across the projectA statement used for the __gap variable, e.g. uint256[47] private __gap, can be declared into a separate contract and reused wherever necessary, instead of being declared directly on multiple contracts. See reference implementation.
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
_disableInitializers() for upgradeable contractsInstead of using an empty constructor with the initializer modifier, OpenZeppelin recommendation is to use _disableInitializers() inside the constructor.
See this post for more information.
OpenZeppelin contracts may be considered draft contracts if they have not received adequate security auditing or are liable to change with future development.
Consider waiting until the contract is finalized. Otherwise, make sure that development team is aware of the risks of using a draft OpenZeppelin contract and accept the risk-benefit trade-off.
For StRSR.setUnstakingDelay() the statement require(rewardPeriod * 2 <= unstakingDelay) can be rewritten as require(rewardPeriod * 2 <= val) and can be moved to top of the function, after the first require and before emitting the event.
The solidity documentation recommends the following layout order for functions:
constructor external public internal private
The instance bellow shows external, then private, then public.
BasketHandler line 188 should have the same indentation level as line 187 and line 189.
Consider naming the imports to improve code readability. E.g. import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; can be rewritten as import {IERC20} from “import “@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol”;
State variables can be saved into a local variable when reading the value more than one on the same function.
E.g. basketsNeeded can be cached in RToken.issue().
Similarly, queue.left can be cached on RToken.vestUpTo():
The comment on L45-46 on Auth.sol seems to be referring to longFreezes instead of longFreeze, since longFreeze is an integer and longFreezes is a mapping.
Also, the comment on L174 for BasketHandler.sol indicates that refreshBasket() is only callable directly by governance. However, the condition on L188-189 allows to be called by anyone as long as the collateral status is disabled and the status is not paused/fronzen.
Furthermore, on RToken.requireValidBUExchangeRate() the comment on L801 indicates the range is between [1e-9, 1e9]. However the checked range at L813 is [1e9, 1e27]. Consider changing the code to improve the readability of the code.
minTradeVolumeThe function Trading.setMinTradeVolume() implements a check to test if the received value is smaller than the minimum. A more consistent approach would be replace min with max for setMinTradeVolume, MIN_TRADE_VOLUME and MinTradeVolumeSet, since the new value has to be smaller than the constant, similarly to how it's implemented on the Trading.setMaxTradeSlippage.
> can be replaced with != for unsigned integer checksWhen checking if a uint is not zero, a more declarative check can be != 0 instead of > 0.
Some functions return named variables, others return explicit values.
Following function returns an explicit value.
Following function return a named variable.
Consider adopting the same approach throughout the codebase to improve the explicitness and readability of the code.
Consider adding NATSPEC documenting all function arguments/return values when present.
E.g. RToken.vest() doesn't contain endId on it's NATSPEC.
Consider using time units e.g. (replace 31536000 with 365 days).
As described on the solidity documentation: "The assert function creates an error of type Panic(uint256). … Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix.
value < 2**96 can be rewritten as value <= type(uint96).max
Using scientific notation for multiples of 10 can improve code readability.
#0 - c4-judge
2023-01-24T23:57:47Z
0xean marked the issue as grade-a
#1 - tbrent
2023-02-06T22:47:25Z
About 2 out of every 3 suggestions here is thoughtful and makes sense
#2 - tbrent
2023-02-06T22:49:11Z
Agree with assessment this is a high-quality report