Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Borrower can frontrun liquidator to block liquidation by adding a tiny premium amount to extend the loan start time and roll over the loan term infinitely #4

Closed
c4-bot-8 opened this issue Dec 14, 2023 · 10 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-33 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-bot-8
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L365
https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/Lien.sol#L46

Vulnerability details

Impact

Borrower can frontrun to block liquidation by adding a tiny premium to update the loan start time

Proof of Concept

Please note the liquidation considition check

   // check for liquidation condition
	///@dev the liquidation condition is that
	///     (EITHER premium is not enough) OR (cutOffTime > startTime AND currentTime > startTime + LOAN_TERM)
	if (
		!((closeCache.tokenFromPremium < liquidateCache.tokenFromOwed ||
			closeCache.tokenToPremium < liquidateCache.tokenToOwed) ||
			(lien.startTime < lps.getRenewalCutoffTime(lien.tokenId) &&
				lien.startTime + LOAN_TERM < block.timestamp))
	) {
		revert Errors.LiquidationNotMet();
	}

if

cutOffTime > startTime AND currentTime > startTime + LOAN_TERM

the loan should be subject to liquidation

however, borrower can keep enough premium to infinitely roll over the LOAN_TERM can adding a tiny amount of premium (even 1 wei of token or 0 amount of premium)

the borrower needs to call addPremium

this would trigger

liens.updatePremium(
	lienKey,
	uint24(((token0Premium + premium0) * Base.BASIS_POINT) / collateral0),
	uint24(((token1Premium + premium1) * Base.BASIS_POINT) / collateral1)
);

which calls this line of code

    function updatePremium(
        mapping(bytes32 => Info) storage self,
        bytes32 lienKey,
        uint24 token0PremiumPortion,
        uint24 token1PremiumPortion
    ) internal {
        self[lienKey].token0PremiumPortion = token0PremiumPortion;
        self[lienKey].token1PremiumPortion = token1PremiumPortion;
        self[lienKey].startTime = uint32(block.timestamp);
    }

the lien start time is reset to block.timestamp

so basically, the third condition

			(lien.startTime < lps.getRenewalCutoffTime(lien.tokenId) &&
				lien.startTime + LOAN_TERM < block.timestamp))

will never to true

then Borrower can frontrun liquidator to block liquidation by adding a tiny premium amount to extend the loan start time and roll over the loan term infnitely

the impact is severe, this borrower can make sure lender never get NFT position orignal liquidity back as long as the premium is sufficient

Tools Used

Manual Review

Recommended Mitigation Steps

one of the option is do not update lien start time when user add new premium

Assessed type

MEV

@c4-bot-8 c4-bot-8 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 14, 2023
c4-bot-8 added a commit that referenced this issue Dec 14, 2023
@c4-judge
Copy link
Contributor

0xleastwood marked the issue as primary issue

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Dec 21, 2023
@wukong-particle
Copy link

Good discovery, but we think this by design is covered by the renewalCutOffTime. Please note that when interacting with addPremium there is a check

https://github.com/code-423n4/2023-12-particle/blob/main/contracts/protocol/ParticlePositionManager.sol#L508-L509

If LP calls reclaimLiquidity https://github.com/code-423n4/2023-12-particle/blob/main/contracts/protocol/ParticlePositionManager.sol#L142, trader won't be able to add premium.

Happy to discuss further along this line. If there's still attack angle, please provide a coded PoC. Thanks!

@JeffCX
Copy link

JeffCX commented Dec 23, 2023

Consider the case when the original borrower that hold NFT does not implement the function reclaimLiquidity.

also,

then Borrower can frontrun liquidator to block liquidation by adding a tiny premium amount to extend the loan start time and roll over the loan term infinitely

before lender calls reclaimLiquidity, every time the borrower add a tiny premium, the position will be extended by 7 days

even when the reclaimLiquidity and the addPremium are called at the same time (borrower frontrun liquidator's reclaimLiquidity to add tiny premium),

the loan. term can still be extended by 7 days

mainly because we are using > instead of >=

        if (lps.getRenewalCutoffTime(lien.tokenId) > lien.startTime) revert Errors.RenewalDisabled();

@wukong-particle
Copy link

Yes, the > vs >= issue is similar to #33, we will fix it!

@c4-judge c4-judge added duplicate-33 and removed primary issue Highest quality submission among a set of duplicates labels Dec 23, 2023
@c4-judge
Copy link
Contributor

0xleastwood marked the issue as duplicate of #33

@0xleastwood
Copy link

It should be noted I think this issue was missing some key points that was outlined in #33. I will give partial credit instead.

@c4-judge
Copy link
Contributor

0xleastwood marked the issue as partial-50

@c4-judge c4-judge added the partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) label Dec 23, 2023
@c4-judge
Copy link
Contributor

0xleastwood changed the severity to 2 (Med Risk)

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Dec 31, 2023
@c4-judge
Copy link
Contributor

0xleastwood marked the issue as full credit

@c4-judge c4-judge removed the partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) label Dec 31, 2023
@c4-judge
Copy link
Contributor

0xleastwood marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Dec 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-33 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

5 participants