setReserveCashBalance can only set less reserves #103
Labels
1 (Low Risk)
Assets are not at risk. State handling, function incorrect as to spec, issues with comments
bug
Something isn't working
sponsor confirmed
Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Comments
code423n4
added
2 (Med Risk)
jeffywu
added
the
sponsor confirmed
|
I think this should be of low severity, a fat finger type of error. The sponsor did not explain why they had this check in the first place though. |
pauliax
added
1 (Low Risk)
jeffywu
added a commit
to notional-finance/contracts-v2
that referenced
this issue
Apr 4, 2022
* Hotfix: disable bitmap currency * Bugfix: disallow changing bitmap currency * Feature: Treasury Action * Feature: switching to transfer / claim ownership * Bugfix: settlement rates set prematurely * Feature: adding patch fix router * Hotfix: setting account context during liquidation calculations * Deployment: adding treasury action to deployment * Misc: removing github actions file * Feature (SOLIDITY): allow nToken to redeem around residuals * Feature (TESTS): allow nToken to redeem around residuals * Deployment: deployed nToken redeem to Kovan * Bugfix (SOLIDITY): incentives calculation fix and migration * Bugfix (TESTS): incentives calculation fix and migration * Refactor: modularizing nToken code * Refactor: simplifying the nToken asset pv calculation * Refactor: handling initialization edge case * Testing: refactor migration code and add fix router * Feature: adding a secondary incentive rewarder * Feature (SOLIDITY): support Aave tokens * Feature (TESTS): support Aave tokens * add router fix message * commenting out some views, code size * refactoring treasury action code * removing unused file * moving address to a separate file * fixing unit test * Treasury cannot claim COMP tokens & COMP tokens are stuck [code-423n4/2022-01-notional-findings#192] * Optimization on _redeemAndTransfer [code-423n4/2022-01-notional-findings#213] * Gas: reserveInternal.subNoNeg(bufferInternal) can be unchecked [code-423n4/2022-01-notional-findings#199] * Revert string > 32 bytes [code-423n4/2022-01-notional-findings#110] * setReserveCashBalance can only set less reserves [code-423n4/2022-01-notional-findings#103] * Gas: When a function uses the onlyManagerContract modifier, use msg.sender instead of treasuryManagerContract [code-423n4/2022-01-notional-findings#98] * Prefix (++i), rather than postfix (i++), increment/decrement operators should be used in for-loops [code-423n4/2022-01-notional-findings#228] * Gas: Missing checks for non-zero transfer value calls [code-423n4/2022-01-notional-findings#94] * Working on token deployer * Refactor compound deployer * Adding notional deployer * Adding mainnet addresses * Adding governance deployer * Adding liquidator deployer * Implementing library checker * Refactor token deployer * Working on contract initialization * Refactoring initializers * Fixing lib deployment logic * Implementing init parameters and various fixes * Initialize markets and updated deployment json * Adding liquidator addresses * fixing tests * Deploying governor and NOTE * Re-deploying NOTE and upgraded router * Adding deployment files * Adding aave lending pool storage * Moving calculation functions out of views * Cleaning up * Updating ABI * Renaming contract * renaming the calculation contract * fixing deployment script * fixing some tests (#29) * fixing some tests * fixes to aave tests * bumping max market index * combining two fixes into patch fix router * updating a number of comments * adding some comments in initi markets * adding safety check in patch fix * re-enabling view methods * fixing potential edge case on claim time * renaming incentives variable * updating rewarder to include the nToken supply change * adding an event for migration incentive * updating abi * adding events and exporting new abi * moving events in treasury action * removing WETH variable * adding fork tests for incentives * adding tests for trading calculations * adding currency id to IRewarder * Bugfix/init markets cash withholding (#32) * adding fork tests for incentives * adding tests for trading calculations * adding currency id to IRewarder * adding test for withholding amount * fixing init markets cash withholding * adding a get present value method for fCash * adding additional checks in fcash liquidation * updating ETH whale * adding additional selector * Fixing patchfix router (#33) * Bugfix/last claim time revert (#34) * adding additional checks in fcash liquidation * updating ETH whale * adding additional selector * implemented a fix for last claim time issue * updating test * Bugfix/init markets at zero rate (#36) * adding additional checks in fcash liquidation * updating ETH whale * adding additional selector * fixing return value for zero rate * Redeploy Kovan (#35) * validated sources * updating some config * Deploying to mainnet (#96) * Deploying to mainnet * Updating mainnet addresses * Adding v2.1 upgrade script Co-authored-by: Tianjie Wei <[email protected]> Co-authored-by: weitianjie2000 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Handle
GeekyLumberjack
Vulnerability details
Impact
There is a fairly decent chance that setReserveCashBalance will mistakenly be set too low. Unlike the case for addresses, the number required is more likely to be manually typed. This will lead to higher chance of a mistype causing unusable reserves. With some functions risks like these are unavoidable. However, in this case, the actions are already performed with a trusted party.
Proof of Concept
require(newBalance < reserveBalance, "cannot increase reserve balance");Tools Used
Manual Analysis
Recommended Mitigation Step
Consider removing
require(newBalance < reserveBalance, "cannot increase reserve balance");https://github.com/code-423n4/2022-01-notional/blob/main/contracts/TreasuryAction.sol#L88
The text was updated successfully, but these errors were encountered: