LoopFi - oualidpro's results

A dedicated lending market for Ethereum carry trades. Users can supply a long tail of Liquid Restaking Tokens (LRT) and their derivatives as collateral to borrow ETH for increased yield exposure.

General Information

Platform: Code4rena

Start Date: 01/05/2024

Pot Size: $12,100 USDC

Total HM: 1

Participants: 47

Period: 7 days

Judge: Koolex

Id: 371

League: ETH

LoopFi

Findings Distribution

Researcher Performance

Rank: 45/47

Findings: 1

Award: $0.00

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository LoopFi

Findings Information

🌟 Selected for report: pamprikrumplikas

Also found by: 0xnev, 0xrex, ParthMandale, Pechenite, SpicyMeatball, ZanyBonzy, caglankaan, chainchief, cheatc0d3, karsar, krisp, oualidpro, peanuts, popeye, slvDev

Awards

0 USDC - $0.00

Labels

bug

grade-b

QA (Quality Assurance)

sponsor acknowledged

sufficient quality report

Q-13

External Links

Link to GitHub issue

Summary

Low Risk Issues

	Issue	Instances
[L‑1]	Missing checks for `address(0)` when assigning values to address state variables	2
[L‑2]	increase/decrease or forceApprove allowance should be used instead of approve	2
[L‑3]	Governance operations should be behind a timelock	4
[L‑4]	Setters should have initial value check	3
[L‑5]	Solidity version 0.8.20 may not work on other chains due to `PUSH0`	1
[L‑6]	Int casting `block.timestamp` can reduce the lifespan of a contract	2
[L‑7]	Unused/empty `receive()`/`fallback()` function	1
[L‑8]	Consider implementing two-step procedure for updating protocol addresses	1
[L‑9]	Privileged functions can create points of failure	5
[L‑10]	Functions calling contracts/addresses with transfer hooks are missing reentrancy guards	1
[L‑11]	Code does not follow the best practice of check-effects-interaction	3
[L‑12]	prevent re-setting a state variable with the same value	3
[L‑13]	Empty receive functions can cause gas issues	1

Total: 29 instances over 13 issues

Non-critical Issues

	Issue	Instances
[NC‑1]	Assembly blocks should have extensive comments	2
[NC‑2]	Constants in comparisons should appear on the left side	12
[NC‑3]	[NatSpec] Natspec description is missing from `contract`	46
[NC‑4]	Contract does not follow the Solidity style guide's suggested layout ordering	1
[NC‑5]	Custom error has no error details	14
[NC‑6]	Consider using `delete` rather than assigning `zero` to clear values	2
[NC‑7]	Empty bytes check is missing	12
[NC‑8]	Events are missing sender information	6
[NC‑9]	Events may be emitted out of order due to reentrancy	4
[NC‑10]	Defining All External/Public Functions in Contract Interfaces	12
[NC‑11]	Function ordering does not follow the Solidity style guide	1
[NC‑12]	`address`s shouldn't be hard-coded	1
[NC‑13]	Imports could be organized more systematically	1
[NC‑14]	Import declarations should import specific identifiers, rather than the whole file	2
[NC‑15]	Inconsistent usage of `require`/`error`	1
[NC‑16]	Large numeric literals should use underscores for readability	1
[NC‑17]	Long lines of code	3
[NC‑18]	Missing event and or timelock for critical parameter change	1
[NC‑19]	Consider using named mappings	3
[NC‑20]	Events that mark critical parameter changes should contain both the old and the new value	2
[NC‑21]	`require()` / `revert()` statements should have descriptive reason strings	1
[NC‑22]	Setters should prevent re-setting of the same value	2
[NC‑23]	NatSpec `@return` argument is missing	3
[NC‑24]	Consider using `SafeTransferLib.safeTransferETH()` or `Address.sendValue()` for clearer semantic meaning	2
[NC‑25]	Empty `receive()`/`payable fallback()` function does not authorize requests	1
[NC‑26]	State variables should have `Natspec` comments	16
[NC‑27]	Contracts should have full test coverage	1
[NC‑28]	Top level pragma declarations should be separated by two blank lines	1
[NC‑29]	Critical functions should be a two step procedure	4
[NC‑30]	Event is missing `indexed` fields	9
[NC‑31]	Missing upgradability functionality	1
[NC‑32]	Constants should be defined rather than using magic numbers	3
[NC‑33]	Use a single file for system wide constants	1
[NC‑34]	Consider using SMTChecker	1
[NC‑35]	Complex function controle flow	2
[NC‑36]	Consider bounding input array length	1
[NC‑37]	[NatSpec] Natspec `@dev` is missing from `contract`	41
[NC‑38]	Contract should expose an `interface`	12
[NC‑39]	[NatSpec] Natspec `@notice` is missing from `contract`	42
[NC‑40]	Contract uses both `require()`/`revert()` as well as custom errors	1
[NC‑41]	`immutable` variable names don't follow the Solidity style guide	1
[NC‑42]	Use the latest Solidity version for better security	1
[NC‑43]	Consider adding formal verification proofs	1
[NC‑44]	Missing zero address check in functions with address parameters	2
[NC‑45]	Use a struct to encapsulate multiple function parameters	1
[NC‑46]	Multiple mappings with same keys can be combined into a single struct mapping for readability	2
[NC‑47]	constructor should emit an event	1
[NC‑48]	Complex functions should include comments	2
[NC‑49]	Make use of Solidiy's `using` keyword	1
[NC‑50]	[Solidity]: All `verbatim` blocks are considered identical by deduplicator and can incorrectly be unified	1
[NC‑51]	[NatSpec] Natspec `@param` is missing from `error`	17
[NC‑52]	Not using the latest version of `prb-math` from dependencies	1
[NC‑53]	Enforcing Lowercase and Underscores Only in the `name` Field of package.json	1
[NC‑54]	Using Low-Level Call for Transfers	2
[NC‑55]	Off-by-one timestamp error	5
[NC‑56]	Incorrect withdraw declaration	1
[NC‑57]	A event should be emitted if a non immutable state variable is set in a constructor	5
[NC‑58]	Returning a struct instead of returning many variables is better	1
[NC‑59]	[Solidity]: Order of Argument Evaluation Disrupted in Non-Expression-Split Code by Optimizer Sequences with FullInliner	1
[NC‑60]	Consider adding validation of user inputs	10
[NC‑61]	Consider using named function calls	8

Total: 337 instances over 61 issues

Low Risk Issues

[L‑1] Missing checks for `address(0)` when assigning values to address state variables

There are 2 instances of this issue:

<details> <summary>see instances</summary>

File: ./src/PrelaunchPoints.sol

99:         exchangeProxy = _exchangeProxy;

337:         owner = _owner;

link , link

</details>

[L‑2] increase/decrease or forceApprove allowance should be used instead of approve

In order to prevent front running, increase/decrease or forceApprove allowance should be used in place of approve where possible

There are 2 instances of this issue:

<details> <summary>see instances</summary>

File: ./src/PrelaunchPoints.sol

231:         lpETH.approve(address(lpETHVault), claimedAmount);

495:         require(_sellToken.approve(exchangeProxy, _amount));

link , link

</details>

[L‑3] Governance operations should be behind a timelock

All critical and governance operations should be protected by a timelock. For example from the point of view of a user, the changing of the owner of a contract is a high risk operation that may have outcomes ranging from an attacker gaining control over the protocol, to the function no longer functioning due to a typo in the destination address. To give users plenty of warning so that they can validate any ownership changes, changes of ownership should be behind a timelock.

There are 4 instances of this issue:

<details> <summary>see instances</summary>

File: ./src/PrelaunchPoints.sol

336:     function setOwner(address _owner) external onlyAuthorized {
337:         owner = _owner;
338: 
339:         emit OwnerUpdated(_owner);
340:     }

364:     function allowToken(address _token) external onlyAuthorized {
365:         isTokenAllowed[_token] = true;
366:     }

372:     function setEmergencyMode(bool _mode) external onlyAuthorized {
373:         emergencyMode = _mode;
374:     }

379:     function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyAuthorized {
380:         if (tokenAddress == address(lpETH) || isTokenAllowed[tokenAddress]) {
381:             revert NotValidToken();
382:         }
383:         IERC20(tokenAddress).safeTransfer(owner, tokenAmount);
384: 
385:         emit Recovered(tokenAddress, tokenAmount);
386:     }

link , link, link, link

</details>

[L‑4] Setters should have initial value check

Setters should have initial value check to prevent assigning wrong value to the variable. Assginment of wrong value can lead to unexpected behavior of the contract.

There are 3 instances of this issue:

<details> <summary>see instances</summary>

File: ./src/PrelaunchPoints.sol

336:     function setOwner(address _owner) external onlyAuthorized {

348:     function setLoopAddresses(address _loopAddress, address _vaultAddress)

372:     function setEmergencyMode(bool _mode) external onlyAuthorized {

link , link, link

</details>

[L‑5] Solidity version 0.8.20 may not work on other chains due to `PUSH0`

The compiler for Solidity 0.8.20 switches the default target EVM version to Shanghai, which includes the new PUSH0 op code. This op code may not yet be implemented on all L2s, so deployment on these chains will fail. To work around this issue, use an earlier EVM version

There are 1 instances of this issue:

<details> <summary>see instances</summary>

File: ./src/PrelaunchPoints.sol

2: pragma solidity 0.8.20;

LoopFi - oualidpro's results

A dedicated lending market for Ethereum carry trades. Users can supply a long tail of Liquid Restaking Tokens (LRT) and their derivatives as collateral to borrow ETH for increased yield exposure.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-13] QA Report - $0.00

Findings Information

Awards

Labels

External Links

Summary

Low Risk Issues

Non-critical Issues

Low Risk Issues

[L‑1] Missing checks for address(0) when assigning values to address state variables

[L‑2] increase/decrease or forceApprove allowance should be used instead of approve

[L‑3] Governance operations should be behind a timelock

[L‑4] Setters should have initial value check

[L‑5] Solidity version 0.8.20 may not work on other chains due to PUSH0

[L‑6] Int casting block.timestamp can reduce the lifespan of a contract

[L‑7] Unused/empty receive()/fallback() function

[L‑8] Consider implementing two-step procedure for updating protocol addresses

[L‑9] Privileged functions can create points of failure

[L‑10] Functions calling contracts/addresses with transfer hooks are missing reentrancy guards

[L‑11] Code does not follow the best practice of check-effects-interaction

[L‑12] prevent re-setting a state variable with the same value

[L‑13] Empty receive functions can cause gas issues

Non-critical Issues

[NC‑1] Assembly blocks should have extensive comments

[NC‑2] Constants in comparisons should appear on the left side

[NC‑3] [NatSpec] Natspec description is missing from contract

[NC‑4] Contract does not follow the Solidity style guide's suggested layout ordering

[NC‑5] Custom error has no error details

[NC‑6] Consider using delete rather than assigning zero to clear values

[NC‑7] Empty bytes check is missing

[NC‑8] Events are missing sender information

[NC‑9] Events may be emitted out of order due to reentrancy

[NC‑10] Defining All External/Public Functions in Contract Interfaces

[NC‑11] Function ordering does not follow the Solidity style guide

[NC‑12] addresss shouldn't be hard-coded

[NC‑13] Imports could be organized more systematically

[NC‑14] Import declarations should import specific identifiers, rather than the whole file

[NC‑15] Inconsistent usage of require/error

[NC‑16] Large numeric literals should use underscores for readability

[NC‑17] Long lines of code

[NC‑18] Missing event and or timelock for critical parameter change

[NC‑19] Consider using named mappings

[NC‑20] Events that mark critical parameter changes should contain both the old and the new value

[NC‑21] require() / revert() statements should have descriptive reason strings

[NC‑22] Setters should prevent re-setting of the same value

[NC‑23] NatSpec @return argument is missing

[NC‑24] Consider using SafeTransferLib.safeTransferETH() or Address.sendValue() for clearer semantic meaning

[NC‑25] Empty receive()/payable fallback() function does not authorize requests

[NC‑26] State variables should have Natspec comments

[NC‑27] Contracts should have full test coverage

[NC‑28] Top level pragma declarations should be separated by two blank lines

[NC‑29] Critical functions should be a two step procedure

[NC‑30] Event is missing indexed fields

[NC‑31] Missing upgradability functionality

[NC‑32] Constants should be defined rather than using magic numbers

[NC‑33] Use a single file for system wide constants

[NC‑34] Consider using SMTChecker

[NC‑35] Complex function controle flow

[NC‑36] Consider bounding input array length

[NC‑37] [NatSpec] Natspec @dev is missing from contract

[NC‑38] Contract should expose an interface

[NC‑39] [NatSpec] Natspec @notice is missing from contract

[NC‑40] Contract uses both require()/revert() as well as custom errors

[NC‑41] immutable variable names don't follow the Solidity style guide

[NC‑42] Use the latest Solidity version for better security

[NC‑43] Consider adding formal verification proofs

[NC‑44] Missing zero address check in functions with address parameters

[NC‑45] Use a struct to encapsulate multiple function parameters

[NC‑46] Multiple mappings with same keys can be combined into a single struct mapping for readability

[NC‑47] constructor should emit an event

[NC‑48] Complex functions should include comments

[NC‑49] Make use of Solidiy's using keyword

[NC‑50] [Solidity]: All verbatim blocks are considered identical by deduplicator and can incorrectly be unified

[NC‑51] [NatSpec] Natspec @param is missing from error

[L‑1] Missing checks for `address(0)` when assigning values to address state variables

[L‑5] Solidity version 0.8.20 may not work on other chains due to `PUSH0`

[L‑6] Int casting `block.timestamp` can reduce the lifespan of a contract

[L‑7] Unused/empty `receive()`/`fallback()` function

[NC‑3] [NatSpec] Natspec description is missing from `contract`

[NC‑6] Consider using `delete` rather than assigning `zero` to clear values

[NC‑12] `address`s shouldn't be hard-coded

[NC‑15] Inconsistent usage of `require`/`error`

[NC‑21] `require()` / `revert()` statements should have descriptive reason strings

[NC‑23] NatSpec `@return` argument is missing

[NC‑24] Consider using `SafeTransferLib.safeTransferETH()` or `Address.sendValue()` for clearer semantic meaning

[NC‑25] Empty `receive()`/`payable fallback()` function does not authorize requests

[NC‑26] State variables should have `Natspec` comments

[NC‑30] Event is missing `indexed` fields

[NC‑37] [NatSpec] Natspec `@dev` is missing from `contract`

[NC‑38] Contract should expose an `interface`

[NC‑39] [NatSpec] Natspec `@notice` is missing from `contract`

[NC‑40] Contract uses both `require()`/`revert()` as well as custom errors

[NC‑41] `immutable` variable names don't follow the Solidity style guide

[NC‑49] Make use of Solidiy's `using` keyword

[NC‑50] [Solidity]: All `verbatim` blocks are considered identical by deduplicator and can incorrectly be unified

[NC‑51] [NatSpec] Natspec `@param` is missing from `error`

[NC‑52] Not using the latest version of `prb-math` from dependencies

[NC‑53] Enforcing Lowercase and Underscores Only in the `name` Field of package.json

#0 - CloudEllie
2024-05-11T17:25:51Z

#1 - 0xd4n1el
2024-05-13T17:11:09Z

#2 - c4-judge
2024-06-03T10:46:09Z