Platform: Code4rena
Start Date: 16/12/2021
Pot Size: $100,000 USDC
Total HM: 21
Participants: 25
Period: 7 days
Judge: alcueca
Total Solo HM: 12
Id: 66
League: ETH
Rank: 11/25
Findings: 3
Award: $2,129.31
🌟 Selected for report: 4
🚀 Solo Findings: 0
heiho1
There are several potential re-entrant functions in contracts/BorrowerOperations.sol:
=> Function addColl() on line 346 is potentially re-entrant as it is external but has no re-entrancy guard declared. This function invokes _adjustTrove() which potentially impacts user debt, collateral top-ups or withdrawals.
=> Function openTrove() on line 207 is potentially re-entrant as it is external but has no re-entrancy guard declared. This function invokes _openTroveInternal() which potentially impacts trove creation, YUSD withdrawals and YUSD gas compensation.
=> Function closeTrove() on line 628 is potentially re-entrant as it is external but has no re-entrancy guard declared. This function invokes troveManagerCached.removeStake(msg.sender) and troveManagerCached.closeTrove(msg.sender) impacting outcomes like debt, rewards and trove ownership.
https://solidity-by-example.org/hacks/re-entrancy/
Slither
Potential solution is a re-entrancy guard similar to https://docs.openzeppelin.com/contracts/4.x/api/security#ReentrancyGuard
#0 - kingyetifinance
2022-01-05T09:02:11Z
@LilYeti: Duplicate with #57
#1 - alcueca
2022-01-15T06:45:58Z
Taking as main
239.0826 USDC - $239.08
heiho1
Functions _requireValidRouterParams() at line 1053 and _requireRouterAVAXIndicesInOrder() at line 1067 are not referenced in the codebase. As internal functions they cannot be externally invoked by non-descendant contracts. Unused code increases contract size to no purpose and should at least be removed to a library so that usage is explicit.
Slither
Remove dead code completely or transfer it to a library for explicit import and use.
#0 - kingyetifinance
2022-01-05T09:03:24Z
@LilYeti: Dupllicate with #99 and is Gas level optimization.
🌟 Selected for report: heiho1
531.2947 USDC - $531.29
heiho1
TroveManagerLiquidations does not inherit contracts/Interfaces/ITroveManagerLiquidations.sol but should. Note that TroveManager.sol does inherit ITroveManager. Decoupling an interface from its implementation can lead to code drift and incomplete or incorrect interfaces/implementations.
Slither
Declare contract as "TroveManagerLiquidations is TroveManagerBase, ITroveManagerLiquidations"
#0 - kingyetifinance
2022-01-05T09:13:43Z
Could be lumped with #188
🌟 Selected for report: heiho1
531.2947 USDC - $531.29
heiho1
TroveManagerRedemptions does not inherit contracts/Interfaces/ITroveManagerRedemptions.sol but should. Note that TroveManager.sol does inherit ITroveManager. Decoupling an interface from its implementation can lead to code drift and incomplete or incorrect interfaces/implementations.
Slither
Declare contract as "TroveManagerRedemptions is TroveManagerBase, ITroveManagerRedemptions"
#0 - kingyetifinance
2022-01-05T09:14:42Z
Could be lumped with #187
🌟 Selected for report: heiho1
97.5905 USDC - $97.59
heiho1
CheckContract is used in ActivePool, BorrowerOperations, CollSurplusPool, DefaultPool, HintHelpers, PriceFeed, SortedTroves, StabilityPool, TroveManager, TroveManagerLiquidations, TroveManagerRedemptions but this is a view function and could easily be implemented as an internal library call. This would result in slightly larger contract bytecode but should be far more gas efficient than an external contract call as is the current case.
https://medium.com/coinmonks/gas-cost-of-solidity-library-functions-dbe0cedd4678
""" Use any of the internal calling methods. We prefer internal library calls, because of the associated class features (see Class Features of Solidity by the same author). Using an external call to a public library function is very expensive, and will only be worth it to avoid including a lot of code into the bytecode for your contract. Using a local contract component is the most expensive option and should be avoided unless essential. """
Slither
Declare CheckContract as an internal library:
https://medium.com/coinmonks/all-you-should-know-about-libraries-in-solidity-dd8bc953eae7
""" Embedded Library: If a smart contract is consuming a library which have only internal functions than EVM simply embeds library into the contract. Instead of using delegate call to call a function, it simply uses JUMP statement(normal method call). There is no need to separately deploy library in this scenario. """
🌟 Selected for report: WatchPug
Also found by: SolidityScan, cccz, heiho1, robee
12.8058 USDC - $12.81
heiho1
Function getCurrentICR() on line 287 is public but never called by TroveManager itself. To optimize gas this function can be marked as external.
""" Explicit function visibility can often provide benefits in terms of smart contract security as well as gas optimization. For example, explicitly labeling external functions forces the function parameter storage location to be set as calldata, which saves gas each time the function is executed. """
https://gus-tavo-guim.medium.com/public-vs-external-functions-in-solidity-b46bcf0ba3ac
Slither
Declare the function to be external.
#0 - kingyetifinance
2022-01-05T09:07:10Z
@LilYeti: Severity should be: Gas optimization
#1 - 0xtruco
2022-01-11T11:16:51Z
Resolve in https://github.com/code-423n4/2021-12-yetifinance/pull/13 Along with #187, #188
#2 - alcueca
2022-01-15T16:50:11Z
Duplicate of #270