Platform: Code4rena
Start Date: 08/07/2021
Pot Size: $50,000 USDC
Total HM: 7
Participants: 13
Period: 7 days
Judge: ghoulsol
Total Solo HM: 5
Id: 18
League: ETH
Rank: 4/13
Findings: 4
Award: $2,077.87
🌟 Selected for report: 1
🚀 Solo Findings: 0
0xRajeev
UniswapV3Oracle contract uses Chainlink’s deprecated API latestAnswer(). Such functions might suddenly stop working if Chainlink stopped supporting deprecated APIs.
Impact: Deprecated API stops working. Prices cannot be obtained. Protocol stops and contracts have to be redeployed.
See similar Low-severity finding L11 from OpenZeppelin's Audit of Opyn Gamma Protocol: https://blog.openzeppelin.com/opyn-gamma-protocol-audit/
See https://docs.chain.link/docs/deprecated-aggregatorinterface-api-reference/#latestanswer.
Manual Analysis
Use V3 interface functions: https://docs.chain.link/docs/price-feeds-api-reference/
#0 - talegift
2021-07-14T14:34:05Z
#75
0xRajeev
The “safe” versions of token transfer/transferFrom as implemented either by OpenZeppelin’s SafeERC20 or Uniswap libraries, use a low-level call and make checks on the return data to handle cases where tokens may not return any value on transfer/transferFrom and make sure that when they do so, it is a boolean whose value is true.
The functions safeTransfer() and safeTransferFrom() implemented here do not do that and instead are wrappers around token transfer/transferFrom checking for their returned boolean values. This will not work as expected, i.e. cause a revert, with tokens that do not return a boolean in these functions.
Manual Analysis
Re-evaluate the function implementations in the context of OZ and Uniswap versions.
#0 - talegift
2021-07-16T02:35:24Z
#67
🌟 Selected for report: 0xRajeev
916.341 USDC - $916.34
0xRajeev
Input validation on key function parameters is a best-practice. Not applying sanity/threshold checks will allow incorrect values to be set accidentally/maliciously and may significantly affect the security/functionality of the protocol.
Except checks on liquidation fees and collaterization factor, the codebase does not have input validation (sanity/threshold checks) on key protocol parameters in setter functions that are callable only by contract owners. Given that the fundamental value proposition of the protocol is to address risk management using isolated lending pairs, it becomes even more important to enforce sanity/threshold validation on protocol parameters to increase confidence in them, reduce risk and prevent accidental/malicious changes that increases risk significantly. The sanity/threshold values may be configurable in a time-delayed manner by owner/governance instead of hardcoding and enforcing unilaterally.
Scenario: Protocol is initialized/configured with absurd/unreasonable (i.e. very low/high) values of critical parameters such as depositLimit, borrowLimit, minBorrowUSD, pool allocation points, totalRewardPerBlock, poolFee, twapPeriod or minObservations. Owners/users fail to check or understand the impact of these absurd values until the protocol’s functionality or profitability is affected significantly.
See similar Major-severity finding from Consensys Diligence Audit of Shell Protocol: https://consensys.net/diligence/audits/2020/06/shell-protocol/#certain-functions-lack-input-validation-routines
Manual Analysis
Enforce sanity/threshold checks in all onlyOwner setters.
#0 - talegift
2021-07-16T02:27:51Z
Severity should be 1.
#1 - ghoul-sol
2021-08-01T21:47:31Z
Agree with sponsor, this is low risk at most.
27.7161 USDC - $27.72
0xRajeev
withdrawBorrowETH() makes a call to _wethWithdrawTo() and then _checkMinReserve() check. However, the check is redundant because _wethWithdrawTo() already performs the same check.
Manual Analysis
Remove the redundant _checkMinReserve check on L111.
#0 - talegift
2021-07-14T14:34:49Z
#26
#1 - ghoul-sol
2021-08-01T20:50:47Z
It's a gas optimisation since there's no exploit here.
0xRajeev
There are two liquidation fees in the protocol: System and Default. Both have default values as well: liqFeeSystemDefault and liqFeeCallerDefault. There is a constraint that the sum of these two should be <= MAX_LIQ_FEES. While this is enforced in the setter functions, this check is missing in the constructor.
Impact: The contract may be deployed with incorrect values of liqFeeSystemDefault and liqFeeCallerDefault such that their sum is > MAX_LIQ_FEES.
Manual Analysis
Add similar check to constructor.
#0 - talegift
2021-07-14T14:43:32Z
#24