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
Rank: 1/47
Findings: 2
Award: $471.42
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: 0xnev
Also found by: 0x04bytes, 0xBugSlayer, 0xJoyBoy03, 0xSecuri, 0xrex, Bigsam, DMoore, Evo, Greed, Kirkeelee, Krace, Pechenite, Rhaydden, SBSecurity, Sajjad, TheFabled, Topmark, XDZIBECX, ZanyBonzy, _karanel, bbl4de, btk, d3e4, gumgumzum, nfmelendez, novamanbg, petarP1998, samuraii77, sandy, shaflow2, sldtyenj12, web3er, y4y, yovchev_yoan
369.7778 USDC - $369.78
https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L262 https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L324
One of the main invariants stated in the contest is the following:
Deposits are active up to the lpETH contract and lpETHVault contract are set
However, there are currently two ways this invariant can be broken, allowing users to gain lpETH without explicitly locking tokens before contracts are set.
convertAllETH by front-running to directly donate ETH and then back-running to claim lpETH via multiple lock positionsThis bypasses the onlyBeforeDate(loopActivation) modifier included in all lock functions. It also potentially allows no cap in lpETH minted and also discourages users from locking ETH in the PrelaunchPoints.sol contract before contract addresses are set (i.e. loopActivation is assigned)
Bundle a transaction of donation and claiming with a previously locked position of wrapped LRT. User can execute the following in one transaction, assuming the user previously has a locked position with any of the allowed LRTs.
claim()/claimAndStake(), claiming all tokens, this swaps users wrapped LRT to ETH via _fillQuote.claimedAmount is set as address(this).balance here, claimedAmountis computed as the addition of ETH receive from LRT swapped + user donated ETHclaimedAmount worth of lpETH is subsequently claimed/staked via lpETH.deposit()/lpETHVault.stake().Sandwich a call to convertAllETH by front-running to directly donate ETH and then back-running to claim lpETH via multiple lock positions
convertAllETH by donating 1 ETH. This sets totalLpETH to be 101 ETH as seen heretotalLpETH : totalSupply computed here is now 1.01 (1.01)convertAllETH by executing claims for all 10 position, each claim will mint 0.01 ETH extra (1.01 lpETH)This scenario is less likely than scenario 1 as it requires user to perfectly back-run convertAllETH() with claims, although it also breaks another invariant of a 1:1 swap
Users that deposit ETH/WETH get the correct amount of lpETH on claim (1 to 1 conversion)
Manual Analysis
claimedAmount to amount of ETH bought from LRT swap (such as buyAmount for uniV3)totalLpETH : totalSupply ratio. If not you can consider setting totalLpETH to totalSupply and allow admin to retrieve any additional ETH donated.Other
#0 - c4-judge
2024-05-15T14:20:52Z
koolexcrypto marked the issue as primary issue
#1 - c4-judge
2024-05-31T09:41:05Z
koolexcrypto changed the severity to 3 (High Risk)
#2 - c4-judge
2024-05-31T09:41:11Z
koolexcrypto marked the issue as satisfactory
#3 - c4-judge
2024-06-05T10:02:41Z
koolexcrypto marked the issue as selected for report
#4 - k4zanmalay
2024-06-06T07:12:21Z
I believe this issue has no impact. It incorrectly assumes that only stakers can mint lpETH, however the sponsor has repeatedly mentioned that the locking feature is only needed to collect points that depend on the duration of the lock, and users can freely mint lpETH without locking their tokens.
#5 - d3e4
2024-06-07T14:56:55Z
@koolexcrypto This report contains two issues:
Scenario 1: Direct transfer of ETH before _claim() to bypass locking requirement.
Scenario 2: Donate ETH to every holder equally.
Firstly, I would like to argue that only Scenario 1 is a High severity issue, whereas Scenario 2 is at best QA. There is no incentive or advantage for the attacker to do this, and allowing someone to distribute funds to everyone equally is a normal feature of a tokenized vault and should not be considered an issue.
Secondly, many of the duplicates fail to mention the exploitative aspect of Scenario 1. Some speak of an accidental transfer to the contract, and others about there randomly being some ETH lying around in the contract. User mistakes are not grounds for a HM issue, and the argument about ETH already present in the contract is either simply false or demonstrates an insufficient or incorrect understanding of the implications of whatever it might have in common with the root cause of Scenario 1. I believe only those reports mentioning a deliberate donation of ETH in order to bypass the locking requirement before calling claim() or claimAndStake() should be considered duplicates of #33. All others should be invalid or QA.
Of the current duplicates of #33 these are the only ones mentioning (however vaguely) a deliberate donation: #58, #57, #56, #55, #53, #51, #48, #46, #40, #38, #37, #34, #33, #32, #30, #27, #26, #25, #22, #20, #19, #6. I believe these should be full duplicates of #33.
Of the remaining current duplicates, #82, #70, #69, #67, #64, #54, #44, #35, #28, #24 only mention a user mistake or a (false, unjustified or unexplained) prior presence of ETH in the contract, and #43, #41, #39, #23, #21, #18 only mention Scenario 2, and #36 is based on the weird claim that "due to market conditions, the actual amount of ETH received from the token swap may differ". I believe these should be invalid or QA.
#6 - koolexcrypto
2024-06-10T09:09:38Z
I believe this issue has no impact. It incorrectly assumes that only stakers can mint lpETH, however the sponsor has repeatedly mentioned that the locking feature is only needed to collect points that depend on the duration of the lock, and users can freely mint lpETH without locking their tokens.
Bypassing the locking duration is a clear impact. The sponsor confirmed the issue.
#7 - koolexcrypto
2024-06-10T09:18:38Z
@koolexcrypto This report contains two issues: Scenario 1: Direct transfer of ETH before
_claim()to bypass locking requirement. Scenario 2: Donate ETH to every holder equally.Firstly, I would like to argue that only Scenario 1 is a High severity issue, whereas Scenario 2 is at best QA. There is no incentive or advantage for the attacker to do this, and allowing someone to distribute funds to everyone equally is a normal feature of a tokenized vault and should not be considered an issue.
Secondly, many of the duplicates fail to mention the exploitative aspect of Scenario 1. Some speak of an accidental transfer to the contract, and others about there randomly being some ETH lying around in the contract. User mistakes are not grounds for a HM issue, and the argument about ETH already present in the contract is either simply false or demonstrates an insufficient or incorrect understanding of the implications of whatever it might have in common with the root cause of Scenario 1. I believe only those reports mentioning a deliberate donation of ETH in order to bypass the locking requirement before calling
claim()orclaimAndStake()should be considered duplicates of #33. All others should be invalid or QA.Of the current duplicates of #33 these are the only ones mentioning (however vaguely) a deliberate donation: #58, #57, #56, #55, #53, #51, #48, #46, #40, #38, #37, #34, #33, #32, #30, #27, #26, #25, #22, #20, #19, #6. I believe these should be full duplicates of #33.
Of the remaining current duplicates, #82, #70, #69, #67, #64, #54, #44, #35, #28, #24 only mention a user mistake or a (false, unjustified or unexplained) prior presence of ETH in the contract, and #43, #41, #39, #23, #21, #18 only mention Scenario 2, and #36 is based on the weird claim that "due to market conditions, the actual amount of ETH received from the token swap may differ". I believe these should be invalid or QA.
Thank you for your feedback.
Generally speaking, I agree with you. I am considering lowering the partial credits given. I won't make it as QA since the exploit scenario is mentioned, those for issues that mention a deliberate donation without the impact of bypassing locking duration. However, for issues that mention it as a user mistake, I think a QA would be fairer.
#8 - k4zanmalay
2024-06-10T09:43:05Z
Bypassing the locking duration is a clear impact. The sponsor confirmed the issue.
@koolexcrypto hi, what do you mean by this? Users can't claim lpETH until the startClaimDate
function claim(address _token, uint8 _percentage, Exchange _exchange, bytes calldata _data) external onlyAfterDate(startClaimDate)
As for the lpETH minting, sponsor commented that it is available for all, no need to lock tokens and implement attack scenarios described in this report, user can call lpETH.deposit directly whenever he wishes.
#9 - koolexcrypto
2024-06-10T20:39:35Z
@k4zanmalay
The core concept is to lock funds for some time to grant the users points based on that
🌟 Selected for report: pamprikrumplikas
Also found by: 0xnev, 0xrex, ParthMandale, Pechenite, SpicyMeatball, ZanyBonzy, caglankaan, chainchief, cheatc0d3, karsar, krisp, oualidpro, peanuts, popeye, slvDev
0 USDC - $0.00
| Number | Title |
|---|---|
| L-01 | Users can self-refer to gain referral extra pointsne of the ERC20 tokens current approval is not zero |
| L-02 | During emergency withdrawals, refferal events can be spoofed |
| L-03 | claiming allow inputs of disallowed token types/WETH |
| I-04 | Include on-chain slippage within _fillQuote() |
| I-05 | Improvement for naming of totalLpETH/totalSupply |
In LoopFi PrelaunchPoints contract, users can lock ETH, WETH and wrapped LRTs into this contract, which will emit events tracked on a backend to calculate their corresponding amount of points. Users can use a referral code encoded as bytes32 that will give the referral extra points. We can presume the event referred in this case is the Locked event emitted within _processLock(). Users with referral codes can self-refer themselves with their personal referral codes to gain extra points aside from the position already locked.
Consider not allowing users with referral code to self lock positions, if not acknowledge it as a design choice
When emergency withdrawals are enabled before vault and lpETH contract is set (i.e. before loopActivation is set within setLoopAddresses), users can still choose to lock ETH, WETH or LRTs.
This way, they can bundle transactions and loop between locking and withdrawing when emergency withdrawal is enabled to spam Locked events, which could potentially flood backend systems.
Additionally, if emergency withdrawals are enabled, it could mean a bug/hack has occured, so it would be wise to prevent further locking of funds.
Consider adding the following check to all lock functions:
if (emergencyMode) revert EmergencyModeEnabled();
Within the_claim function, user can input _token addresses that are not allowed. This can allow users to utilize any donated ETH to mint additional lpETH without a prior locking of funds by donating ETH directly into contract and subsequently calling claim()/claimAndStake(), bypassing the onlyBeforeDate(loopActivation) modifier within lock functions. However, user will risk their ETH donated being front-runned and claimed by other users claiming.
Consider adding the following check to _claim().
if (!isTokenAllowed[token]) revert TokenNotAllowed();
_fillQuoteIt could be a good improvement to include an on-chain slippage as a input for users for better clarity and usage within the claim functions directly when executing swaps for wrapped LRTs to ETH when claiming lpETH.
Consider adding a input variable representing slippage for claim/claimAndStake() that can be checked within _fillQuote()
Within the contract, totalSupply tracks all the ETH/WETH locked while totalLpETH is the amount received after convertAllETH() is executed. This variable names are abit misleading and could be refactored for better clarity
Consider changing names from
totalSupply to totalETHLockedtotalLpETH to totalLpETHConvertedFromETH#0 - CloudEllie
2024-05-11T17:26:25Z
#1 - 0xd4n1el
2024-05-13T17:05:48Z
Good report
#2 - c4-judge
2024-06-03T10:44:27Z
koolexcrypto marked the issue as grade-b