Platform: Code4rena
Start Date: 23/09/2021
Pot Size: $50,000 USDC
Total HM: 5
Participants: 14
Period: 7 days
Judge: ghoulsol
Total Solo HM: 3
Id: 32
League: ETH
Rank: 3/14
Findings: 4
Award: $10,356.99
🌟 Selected for report: 15
🚀 Solo Findings: 0
5947.4124 USDC - $5,947.41
0xRajeev
In _supplyCreditUni(), the last argument of _convertTokenValues() on L674 being _priceB instead of _priceA in the calculation of supplyB is a typo (should be _priceA) and therefore miscalculates supplyB, creditB, creditUni and therefore totalAccountSupply in function accountHealth() which affects the health of account/protocol determination that is used across all borrows/withdrawals/transfers/liquidations in the protocol. This miscalculation significantly affects all calculations in protocol and could therefore cause protocol insolvency.
Manual Analysis
Change the last argument of _convertTokenValues() from _priceB to _priceA on L674.
1070.5342 USDC - $1,070.53
0xRajeev
The 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/
This was a Medium-severity finding even in the previous version of WildCredit contest as well: https://github.com/code-423n4/2021-07-wildcredit-findings/issues/75 where it was reported that "latestAnswer method will return the last value, but you won’t be able to check if the data is fresh. On the other hand, calling the method latestRoundData allow you to run some extra validations”
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-10-01T12:53:25Z
We'll remove dependence on Chainlink completely.
🌟 Selected for report: 0xRajeev
1321.6472 USDC - $1,321.65
0xRajeev
While all other contracts use Solidity compiler version 0.8.6, UniswapV3Helper uses version 0.7.5 because as noted in the documentation “it's not easy to port to 0.8.” This however means that contracts/functions in this file do not get the automatic arithmetic checks like other files.
While there are no obvious overflow/underflows observed, it is safer to use SafeMath for arithmetic operations in this file.
Manual Analysis
Use SafeMath
🌟 Selected for report: 0xRajeev
Also found by: GalloDaSballo
356.8447 USDC - $356.84
0xRajeev
The desired constraint is for minRate to be < lowRate and is checked as such in setMinRate setter. However, lowRate can be changed at any/later point by using the setLowRate setter which can set it to a value lower than minRate because that constraint is not checked/enforced in setLowRate setter.
Manual Analysis
Check that lowRate is > minRate in setLowRate setter
#0 - talegift
2021-10-02T04:25:29Z
Requires admin error. Lower severity to 0.
#1 - ghoul-sol
2021-10-12T04:51:58Z
Sponsor is correct, however I see a logical error here. Low risk is correct.
594.7412 USDC - $594.74
0xRajeev
The desired constraint is for highRate to be > lowRate and is checked as such in setLowRate setter. However, highRate can be changed at any/later point by using the setHighRate setter which can set it to a value lower than lowRate because that constraint is not checked/enforced in setHighRate setter.
Manual Analysis
Check that highRate is > lowRate in setHighRate setter. If both need to be changed, then provide a combined setter for all three min/low/high rates which enforces the threshold checks on all three new values at once.
#0 - talegift
2021-10-02T05:00:35Z
Required an admin error. Set severity to 0.
#1 - ghoul-sol
2021-10-12T04:53:04Z
For the same reason as #61, keeping low risk
🌟 Selected for report: 0xRajeev
116.225 USDC - $116.23
0xRajeev
There are numerous places across contracts where the same state variables (addresses or other types) are read multiple times or the same external calls are made multiple times within a function. Caching state variables or results from external calls in local/memory variables avoids SLOADs and CALLs to save gas. Warm SLOADs cost 100 gas and CALLs cost 2600 gas after Berlin upgrade. MLOADs cost only 3 gas units.
Cache pools[_token].pairToken and poolFee: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/UniswapV3Oracle.sol#L85-L95
Cache minRate, lowRate, highRate and targetUtilization: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/InterestRateModel.sol#L73-L83
Cache tokenA & tokenB: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/LendingPair.sol#L88-L99
Cache tokenA & tokenB: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/LendingPair.sol#L282-L318
Cache lendingController: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/LendingPair.sol#L299-L303
Cache lendingController: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/LendingPair.sol#L334-L336
Cache tokenA & tokenB: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/LendingPair.sol#L335-L346
Cache uniPosition[_account]: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/LendingPair.sol#L450-L458
Cache lendingController, uniV3Helper, tokenA and tokenB: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/LendingPair.sol#L452-L465
Cache totalDebtAmount[_token]: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/LendingPair.sol#L522-L525
Cache pendingOwner: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/external/Ownable.sol#L36-L38
Cache lendingController: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/PairFactory.sol#L58-L66
Manual Analysis
Cache state variables or results from external calls in local/memory variables to save gas.
🌟 Selected for report: 0xRajeev
116.225 USDC - $116.23
0xRajeev
The zero-address checks in LendingPair initialiaze() are redundant if the only expected flow is from the PairFactory contract which also would have performed the same check before the call to this function.
Manual Analysis
Evaluate flow and remove one of the two checks.
🌟 Selected for report: 0xRajeev
116.225 USDC - $116.23
0xRajeev
Adding a require on positionID not being 0 in withdrawUniPosition() will avoid gas usage from external call and other following logic, in cases where the msg.sender does not have a valid uniPosition.
Manual Analysis
Add a require on positionID != 0 in the beginning of withdrawUniPosition()
52.3013 USDC - $52.30
0xRajeev
For all functions taking user input of amount, requiring amount > 0 at the beginning of the function will save gas from avoiding execution of further logic for accidentally triggered zero amount transactions. This is a best-practice.
and many others.
Manual Analysis
Add require (_amount > 0) at the beginning of the functions that accept _amount as parameter.
🌟 Selected for report: 0xRajeev
116.225 USDC - $116.23
0xRajeev
Given the use of Solidity compiler >= 0.8.0, there are default arithmetic checks for mathematical operations which consume additional gas for such checks internally. In expressions where we are absolutely sure of no overflows/underflows, one can use the unchecked{} primitive to wrap such expressions to avoid checks and save gas.
For example, given the check on L419, we can use the unchecked{} directive on L420 because repayAmount is guaranteed to be <= _amount due to the prior check.
Manual Analysis
Use unchecked{} primitive to wrap arithmetic expressions where we are absolutely sure of no overflows/underflows. This avoids built-in checks and saves gas.
🌟 Selected for report: 0xRajeev
116.225 USDC - $116.23
0xRajeev
Moving _checkBorrowEnabled and _checkDepositsEnabled checks to before the mint function call will save gas for invalid calls and is also a best practice to move validation checks closest to the interface.
Manual Analysis
Move _checkBorrowEnabled and _checkDepositsEnabled checks to before the mint function calls.
52.3013 USDC - $52.30
0xRajeev
Unused parameter removal of _account in _checkBorrowLimits() can save gas similar to _checkDepositLimit().
Manual Analysis
Remove unused parameter _account in _checkBorrowLimits().
🌟 Selected for report: 0xRajeev
116.225 USDC - $116.23
0xRajeev
Using msg.sender or cached local variables in emits instead of state variables set to msg.sender values saves gas by avoiding expensive SLOADs that cost 2100 gas after Berlin upgrade.
Use msg.sender instead of owner: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/external/Ownable.sol#L17
Use cached local variable instead of pendingOwner: https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/external/Ownable.sol#L36-L38
Manual Analysis
Use msg.sender or cached locals in emits instead of state variables
🌟 Selected for report: 0xRajeev
116.225 USDC - $116.23
0xRajeev
There is an unnecessary SSTORE by setting pendingOwner state variable to zero address, when this is not checked anywhere but only prevents multiple accepts from the same pendingOwner which is not harmful.
Manual Analysis
Remove the setting of pendingOwner state variable to zero address.
🌟 Selected for report: 0xRajeev
116.225 USDC - $116.23
0xRajeev
The bool initialized gets packed with address variable underlying (boolean is internally uint8 and address is 20 bytes, both of which fit in a 32B slot) and requires extra bytecode for masking whenever underlying is used while initialized is used rarely and only inside the initialize() function.
Manual Analysis
Move declaration of bool initialized to after string symbol.
31.3808 USDC - $31.38
0xRajeev
All five address-type state variables in PairFactory can be made immutable (because they are only ever initialized in the constructor) to save storage slots and gas because SLOADs are not needed anymore for their reads as their usages are replaced at construction time by their fixed values.
Manual Analysis
Convert the five state variables to immutable
#0 - talegift
2021-09-30T04:38:32Z
Duplicate of #1