Gas: When a function uses the onlyManagerContract modifier, use msg.sender instead of treasuryManagerContract
#98
Labels
bug
Something isn't working
G (Gas Optimization)
invalid
This doesn't seem right
sponsor confirmed
Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Comments
jeffywu
added
the
sponsor confirmed
|
Similar to #97, I am grouping these together. |
pauliax
added
invalid
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
Dravee
Vulnerability details
Impact
msg.sendercosts 2 gas (CALLER opcode)treasuryManagerContractcosts 100 gas (SLOAD opcode)The
onlyManagerContractmodifier already checks thatmsg.sender == treasuryManagerContract.Proof of Concept
Instances include:
The 2nd call L140 is actually in a private function (
_redeemAndTransfer()) that doesn't use theonlyManagerContractmodifier. However, the only place where this function is called is insidetransferReserveToTreasury()which does use theonlyManagerContractmodifier. This also means that_redeemAndTransfer()could be inlined intransferReserveToTreasury()to save some gas.Tools Used
VS Code
Recommended Mitigation Steps
When a function uses the
onlyManagerContractmodifier, usemsg.senderinstead oftreasuryManagerContract.I'd advise againt inlining
_redeemAndTransfer()to save gas (for the sake of clarity), but it's your choice.The text was updated successfully, but these errors were encountered: