Platform: Code4rena
Start Date: 15/06/2023
Pot Size: $79,800 USDC
Total HM: 14
Participants: 6
Period: 14 days
Judge: 0xean
Total Solo HM: 11
Id: 248
League: ETH
Rank: 4/6
Findings: 2
Award: $0.00
🌟 Selected for report: 2
🚀 Solo Findings: 2
🌟 Selected for report: RaymondFam
Data not available
https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/plugins/trading/DutchTrade.sol#L160 https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/RevenueTrader.sol#L46 https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/BackingManager.sol#L81
notTradingPausedOrFrozen that is turned on and off during an open Dutch trade could have the auction closed with a lower price depending on the timimg, leading to lesser capability to boost the Rtoken and/or stRSR exchange rates as well as a weakened recollaterization.
Here's the scenario:
A 30 minute Dutch trade is opened by the Revenue trader selling a suplus token for Rtoken.
Shortly after the price begins to decrease linearly, Alice calls bid(). As can be seen in line 160 of the code block below, settleTrade() is externally called on the origin, RevenueTrader.sol in this case:
function bid() external returns (uint256 amountIn) { require(bidder == address(0), "bid already received"); // {qBuyTok} amountIn = bidAmount(uint48(block.timestamp)); // enforces auction ongoing // Transfer in buy tokens bidder = msg.sender; buy.safeTransferFrom(bidder, address(this), amountIn); // status must begin OPEN assert(status == TradeStatus.OPEN); // settle() via callback 160: origin.settleTrade(sell); // confirm callback succeeded assert(status == TradeStatus.CLOSED); }
pauseTrading() invoked by a PAUSER, and denied on line 46 of the function below:function settleTrade(IERC20 sell) public override(ITrading, TradingP1) 46: notTradingPausedOrFrozen returns (ITrade trade) { trade = super.settleTrade(sell); // nonReentrant distributeTokenToBuy(); // unlike BackingManager, do _not_ chain trades; b2b trades of the same token are unlikely }
As the auction is nearing to endTime, the PAUSER calls unpauseIssuance().
Bob, the late comer, upon seeing this, proceeds to calling bid() and gets the sell token for a price much lower than he would initially expect before the trading pause.
Manual
Consider removing notTradingPausedOrFrozen from the function visibility of RevenueTrader.settleTrade and BackingManager.settleTrade. This will also have a good side effect of allowing the settling of a Gnosis trade if need be. Collectively, the settled trades could at least proceed to helping boost the RToken and/or stRSR exchange rates that is conducive to the token holders redeeming and withdrawing. The same shall apply to enhancing recollaterization, albeit future tradings will be halted if the trading pause is still enabled.
Timing
#0 - 0xean
2023-06-29T21:15:22Z
This also seems like QA. It outlines a very specific set of events that are very unlikely to occur during production scenarios and would additionally come down to admin misconfiguration / mismanagement. will wait for sponsor comment, but most likely downgrade to QA
#1 - 0xean
2023-06-29T21:25:21Z
- The PAUSER role should be assigned to an address that is able to act quickly in response to off-chain events, such as a Chainlink feed failing. It is acceptable for there to be false positives, since redemption remains enabled.
it is good to consider this quote from the documentation stating that pausing may have false positives.
#2 - tbrent
2023-07-03T17:34:44Z
We believe a malicious pauser attack vector is dangerous enough that the issue is Medium and deserves a mitigation. Agree with suggested mitigation.
#3 - c4-sponsor
2023-07-03T17:34:53Z
tbrent marked the issue as sponsor confirmed
#4 - c4-judge
2023-07-12T19:21:33Z
0xean marked the issue as satisfactory
🌟 Selected for report: RaymondFam
Data not available
https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/plugins/trading/GnosisTrade.sol#L202 https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/Broker.sol#L119-L123
GnosisTrade and DutchTrade are two separate auction systems where the failing of either system should not affect the other one. The current design will have Broker.sol disabled when reportViolation is invoked by GnosisTrade.settle() if the auction's clearing price was below what we assert it should be.
broker.reportViolation();
function reportViolation() external notTradingPausedOrFrozen { require(trades[_msgSender()], "unrecognized trade contract"); emit DisabledSet(disabled, true); disabled = true; }
Consequently, both BackingManager and RevenueTrader (rsrTrader and rTokenTrader) will not be able to call openTrade():
function openTrade(TradeKind kind, TradeRequest memory req) external returns (ITrade) { require(!disabled, "broker disabled"); ...
till it's resolved by the governance:
function setDisabled(bool disabled_) external governance { emit DisabledSet(disabled, disabled_); disabled = disabled_; }
The following Trading.trytrade() as inherited by BackingManager and RevenueTrader will be denied on line 121, deterring recollaterization and boosting of Rtoken and stRSR exchange rate. The former deterrence will have Rtoken.redeemTo and StRSR.withdraw (both requiring fullyCollateralized) denied whereas the latter will have the Rtoken and stRSR holders divested of intended gains.
function tryTrade(TradeKind kind, TradeRequest memory req) internal returns (ITrade trade) { /* */ IERC20 sell = req.sell.erc20(); assert(address(trades[sell]) == address(0)); IERC20Upgradeable(address(sell)).safeApprove(address(broker), 0); IERC20Upgradeable(address(sell)).safeApprove(address(broker), req.sellAmount); 121: trade = broker.openTrade(kind, req); trades[sell] = trade; tradesOpen++; emit TradeStarted(trade, sell, req.buy.erc20(), req.sellAmount, req.minBuyAmount); }
Manual
Consider having the affected code refactored as follows:
function openTrade(TradeKind kind, TradeRequest memory req) external returns (ITrade) { - require(!disabled, "broker disabled"); address caller = _msgSender(); require( caller == address(backingManager) || caller == address(rsrTrader) || caller == address(rTokenTrader), "only traders" ); // Must be updated when new TradeKinds are created if (kind == TradeKind.BATCH_AUCTION) { + require(!disabled, "Gnosis Trade disabled"); return newBatchAuction(req, caller); } return newDutchAuction(req, ITrading(caller)); }
This will have the Gnosis Trade conditionally denied while still allowing the opening of Dutch Trade.
Context
#0 - 0xean
2023-06-29T22:11:31Z
This currently mostly reads like a design suggestion. I can see the merits of disabling the entire broker in the scenario where the invariant has been violated. Probably best as QA, but will allow for sponsor comment before downgrading.
#1 - tbrent
2023-07-03T17:28:09Z
We think this should be kept as Medium. It's a good design suggestion that otherwise could lead to the protocol not trading for the length of the governance cycle. This matters when it comes to selling defaulted collateral.
#2 - c4-sponsor
2023-07-04T22:08:15Z
tbrent marked the issue as sponsor confirmed
#3 - c4-judge
2023-07-12T19:22:17Z
0xean marked the issue as satisfactory
🌟 Selected for report: 0xA5DF
Also found by: RaymondFam, carlitox477, hihen, ronnyx2017, rvierdiiev
Data not available
Token transfer to the origin in DutchTrade.sol should be refactored as follows like it has been implemented in GnosisTrade.sol to avoid zero token transfer that could be due to auction ending with no bid or the sellAmount fully sold. Additionally, there might be some ERC20 token that could revert on zero value transfer:
// Transfer balances back to origin - buy.safeTransfer(address(origin), boughtAmt); + if (boughtAmt > 0) buy.safeTransfer(address(origin), boughtAmt); - sell.safeTransfer(address(origin), sellBal); + if (sellBal > 0) sell.safeTransfer(address(origin), sellBal);
Registering a new asset in AssetRegistry.sol when the system is frozen would skip giving RToken max allowance over the registered token erc20 by BackingManager.sol:
if (!main.frozen()) { backingManager.grantRTokenAllowance(erc20); }
This could inadvertently halt redemptions when atempting to send withdrawal if BackingManager.grantRTokenAllowance() has not been separately/manually called on the affected erc20.
Consider soft coding GAS_TO_RESERVE in AssetRegistry.sol by adopting a variable that can be updated by the contract owner or governance mechanism. That way, it could be adjusted over time to accommodate changes in Ethereum's gas pricing in the future.
uint256 public constant GAS_TO_RESERVE = 900000; // just enough to disable basket on n=128
Consider implementing batch functions for setBackupConfig() and getBackupConfig() in BasketHandler.sol for an option to handle multiple targetName.
- // from this trade's acution will all eventually go to origin. + // from this trade's auction will all eventually go to origin.
- // tokens. If we actually *get* a worse clearing that worstCasePrice, we consider it an error in + // tokens. If we actually *get* a worse clearing than worstCasePrice, we consider it an error in
- /// @return The status of this collateral asset. (Is it defaulting? Might it soon?) + /// @return The status of this collateral asset. (Is it defaulting? Might it be soon?)
- // lastPayoutBal' = rToken.balanceOf'(this) (balance now == at end of pay leriod) + // lastPayoutBal' = rToken.balanceOf'(this) (balance now == at end of pay period)
- // _delegates[account] is the address of the delegate that `accountt` has specified + // _delegates[account] is the address of the delegate that `account` has specified
As denoted in the Moralis academy article:
"... If a node receives a new chain that’s longer than its current active chain of blocks, it will do a chain reorg to adopt the new chain, regardless of how long it is."
Depending on the outcome, if it ended up placing the transaction earlier than anticipated, the bidder might end up paying more than the intended bidAmount.
(Note: On Ethereum this is unlikely but this is meant for contracts going to be deployed on any compatible EVM chain many of which are frequently reorganized. A maximum bidAmount protection may have to be introduced then.)
furnace.melt() could be missed in atomic transactionThe else if block of RevenueTrader.manageToken could have furnace.melt() dodged:
} else if (assetRegistry.lastRefresh() != uint48(block.timestamp)) { // Refresh everything only if RToken is being traded assetRegistry.refresh(); furnace.melt(); }
if an atomic transaction chaining assetRegistry.refresh() is entailed since this is the only place where lastRefresh is updated on line 64:
function refresh() public { // It's a waste of gas to require notPausedOrFrozen because assets can be updated directly uint256 length = _erc20s.length(); for (uint256 i = 0; i < length; ++i) { assets[IERC20(_erc20s.at(i))].refresh(); } basketHandler.trackStatus(); 64: lastRefresh = uint48(block.timestamp); // safer to do this at end than start, actually }
|| should be replaced with &&This concerns require checks of setBatchAuctionLength() and setDutchAuctionLength() in Broker.sol as using || could allow the auction length set to zero that will correspondingly make newBatchAuction() and newDutchAuction() revert:
require( - newAuctionLength == 0 || + newAuctionLength == 0 && (newAuctionLength >= MIN_AUCTION_LENGTH && newAuctionLength <= MAX_AUCTION_LENGTH), "invalid batchAuctionLength" );
require( - newAuctionLength == 0 || + newAuctionLength == 0 && (newAuctionLength >= MIN_AUCTION_LENGTH && newAuctionLength <= MAX_AUCTION_LENGTH), "invalid dutchAuctionLength" );
Consider implementing a lower amount limit when issuing or redeeming RToken to avoid dealing with dusts.
Additionally, under very rare circumstances, haircuts leading to lower exchange rate of RToken to Collateral could technically have users minting zero amount of RToken due to precision issue while having had to still send in a series of dust collaterals because of CEIL rounding on basketHandler.quote. The same shall also apply to redemption with dust RToken burned and zero collateral received due to FLOOR rounding.
Unlike quoteCustomRedemption() that uses safeMulDivFloor() when calculating quantities, quote() uses safeMul() after the division entailed in _quantity().
Consider having the affected code refactored as follows:
// {qTok} = {tok/BU} * {BU} * {tok} * {qTok/tok} - quantities[i] = _quantity(basket.erc20s[i], coll) - .safeMul(amount, rounding) + quantities[i] = amount + .safeMul(_quantity(basket.erc20s[i], coll), rounding) .shiftl_toUint( int8(IERC20Metadata(address(basket.erc20s[i])).decimals()), rounding );
#0 - c4-judge
2023-07-12T19:58:21Z
0xean marked the issue as grade-a
🌟 Selected for report: 0xA5DF
Also found by: RaymondFam, carlitox477, hihen
Data not available
Consider clearing a dynamic array in reverse order from the end of the set to avoid the need to shift elements after each removal.
For instance, the code line below may be refactored as follows:
- while (targetNames.length() > 0) targetNames.remove(targetNames.at(0)); + while (targetNames.length() > 0) targetNames.remove(targetNames.at(targetNames.length() - 1));
assets[asset.erc20()] == asset of AssetRegistry._register() will assuredly return false on line 189 when invoking _registerIgnoringCollisions(). Consider removing this conditon:
function _register(IAsset asset) internal returns (bool registered) { require( - !_erc20s.contains(address(asset.erc20())) || assets[asset.erc20()] == asset, + !_erc20s.contains(address(asset.erc20())), "duplicate ERC20 detected" ); registered = _registerIgnoringCollisions(asset); } function _registerIgnoringCollisions(IAsset asset) private returns (bool swapped) { IERC20Metadata erc20 = asset.erc20(); if (_erc20s.contains(address(erc20))) { 189: if (assets[erc20] == asset) return false; else emit AssetUnregistered(erc20, assets[erc20]); } else { _erc20s.add(address(erc20)); } ...
It's clearly evident that baskets.bottom will end up assigned FIX_ZERO or fixMin(baskets.bottom, inBUs) in the for loop of the function logic below. As such, pre-assigning it with FIX_MAX is unnecessary:
function basketsHeldBy(address account) public view returns (BasketRange memory baskets) { uint256 length = basket.erc20s.length; if (length == 0 || disabled) return BasketRange(FIX_ZERO, FIX_MAX); - baskets.bottom = FIX_MAX; + baskets.bottom = FIX_MAX; for (uint256 i = 0; i < length; ++i) { ICollateral coll = assetRegistry.toColl(basket.erc20s[i]); if (coll.status() == CollateralStatus.DISABLED) return BasketRange(FIX_ZERO, FIX_MAX); uint192 refPerTok = coll.refPerTok(); // If refPerTok is 0, then we have zero of coll's reference unit. // We know that basket.refAmts[basket.erc20s[i]] > 0, so we have no baskets. if (refPerTok == 0) return BasketRange(FIX_ZERO, FIX_MAX); // {tok/BU} = {ref/BU} / {ref/tok}. 0-division averted by condition above. uint192 q = basket.refAmts[basket.erc20s[i]].div(refPerTok, CEIL); // q > 0 because q = (n).div(_, CEIL) and n > 0 // {BU} = {tok} / {tok/BU} uint192 inBUs = coll.bal(account).div(q); baskets.bottom = fixMin(baskets.bottom, inBUs); baskets.top = fixMax(baskets.top, inBUs); } }
The first three lines of code in redemptionAvailable() may be combined into one code line like it has been done so in issuanceAvailable() and then merged into the if block considering there is no gas saving benefit in caching totalSupply() and the returned value of redemptionThrottle.hourlyLimit(supply) that will only be used once in the function logic:
/// @return {qRTok} The maximum issuance that can be performed in the current block function issuanceAvailable() external view returns (uint256) { return issuanceThrottle.currentlyAvailable(issuanceThrottle.hourlyLimit(totalSupply())); } /// @return available {qRTok} The maximum redemption that can be performed in the current block function redemptionAvailable() external view returns (uint256 available) { uint256 supply = totalSupply(); uint256 hourlyLimit = redemptionThrottle.hourlyLimit(supply); available = redemptionThrottle.currentlyAvailable(hourlyLimit); if (supply < available) available = supply; }
Identical require code block in the following two functions may be merged into a modifier or simply have both function custom merged into one:
/// @custom:governance function setIssuanceThrottleParams(ThrottleLib.Params calldata params) public governance { require(params.amtRate >= MIN_THROTTLE_RATE_AMT, "issuance amtRate too small"); require(params.amtRate <= MAX_THROTTLE_RATE_AMT, "issuance amtRate too big"); require(params.pctRate <= MAX_THROTTLE_PCT_AMT, "issuance pctRate too big"); emit IssuanceThrottleSet(issuanceThrottle.params, params); issuanceThrottle.params = params; } /// @custom:governance function setRedemptionThrottleParams(ThrottleLib.Params calldata params) public governance { require(params.amtRate >= MIN_THROTTLE_RATE_AMT, "redemption amtRate too small"); require(params.amtRate <= MAX_THROTTLE_RATE_AMT, "redemption amtRate too big"); require(params.pctRate <= MAX_THROTTLE_PCT_AMT, "redemption pctRate too big"); emit RedemptionThrottleSet(redemptionThrottle.params, params); redemptionThrottle.params = params; }
You can have further advantages in term of gas cost by simply using named return values as temporary local variable.
For instance, the code block below may be refactored as follows:
/// @return {qRSR} The balance of RSR that this contract owns dedicated to future RSR rewards. - function rsrRewards() internal view returns (uint256) { + function rsrRewards() internal view returns (uint256 _rsrRewards) { _rsrRewards = rsr.balanceOf(address(this)) - stakeRSR - draftRSR; }
|| costs less gas than its equivalent &&Rule of thumb: (x && y) is (!(!x || !y))
Even with the 10k Optimizer enabled: ||, OR conditions cost less than their equivalent &&, AND conditions.
As an example, the instance below may be refactored as follows:
- if (erc20 != IERC20(address(rToken)) && tokenToBuy != IERC20(address(rToken))) { + if (!(erc20 == IERC20(address(rToken)) || tokenToBuy == IERC20(address(rToken)))) {
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.17", 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.
Consider having the logic of a modifier embedded through a private function to reduce contract size if need be. A private visibility that saves more gas on function calls than the internal visibility is adopted because the modifier will only be making this call inside the contract.
For instance, the modifier instance below may be refactored as follows:
+ function _notTradingPausedOrFrozen() private view { + require(!main.tradingPausedOrFrozen(), "frozen or trading paused"); + } modifier notTradingPausedOrFrozen() { - require(!main.tradingPausedOrFrozen(), "frozen or trading paused"); + _notTradingPausedOrFrozen(); _; }
#0 - c4-judge
2023-07-12T20:01:49Z
0xean marked the issue as grade-a