Platform: Code4rena
Start Date: 30/05/2023
Pot Size: $24,170 USDC
Total HM: 10
Participants: 5
Period: 3 days
Judge: hansfriese
Total Solo HM: 2
Id: 244
League: ETH
Rank: 5/5
Findings: 6
Award: $0.00
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: rbserver
Also found by: adriro, bin2chen, d3e4, minhquanym
Data not available
The lender can be prevented from liquidating the borrower, at negligible cost to the borrower.
When the borrower is insolvent the lender can liquidate him by withdrawEthWithInterest(lien, lienId). The parameters are verified by the modifier validateLien() which checks that the lien is hashed to the same digest as is stored at liens[lienId].
The issue is that the borrower than frontrun the lender call to withdrawEthWithInterest() with a call to addCredit() providing only 1 wei, even though he is deeply insolvent. addCredit() changes the value credit in the lien which updates the stored digest. This causes the lien validation to revert in the lender's call to withdrawEthWithInterest().
This way the borrower can avoid liquidation, without having to top up his credit to meet his owed interest. The borrower can thus limit his loss while still having access to the NFT, hoping to trade it later during more favourable market conditions.
Require the amount provided in addCredit() to make the borrower solvent.
DoS
#0 - c4-judge
2023-06-06T09:43:00Z
hansfriese marked the issue as satisfactory
#1 - c4-judge
2023-06-06T10:13:05Z
hansfriese marked the issue as duplicate of #31
#2 - c4-sponsor
2023-06-07T00:48:39Z
wukong-particle marked the issue as sponsor acknowledged
#3 - wukong-particle
2023-06-07T00:48:41Z
Judge is correct, indeed duplication.
#4 - c4-sponsor
2023-06-07T00:51:35Z
wukong-particle marked the issue as sponsor confirmed
#5 - wukong-particle
2023-06-12T22:41:23Z
this actually should be duplicate of https://github.com/code-423n4/2023-05-particle-findings/issues/16 (1 wei addCredit DoS), fixed with https://github.com/Particle-Platforms/particle-exchange-protocol/pull/14 (addCredit needs exceed minimum 0.01 ETH )
🌟 Selected for report: bin2chen
Also found by: d3e4, minhquanym, rbserver
Data not available
The lender might lose or be robbed of his supplied NFT.
A lender can withdraw the NFT in his lien if it is not currently on loan by calling withdrawNftWithInterest(). This is checked by relying on IRC721.safeTransferFrom() to revert if the NFT is not in the contract, since it is assumed that if one "can withdraw [it] means NFT is currently in contract without active loan".
This assumption is wrong! The NFT might have returned to the contract by being supplied in another lien. This means that the first lien has an active loan and should NOT be able to withdraw its NFT, while the second lien is inactive and loses its NFT.
This can even be exploited to set a trap to directly steal an NFT.
Suppose a lender has supplied an NFT which is then borrowed and sold on the market. The attacker buys the same NFT (possibly directly from the borrower) and sets up a lien of his own and then swaps it himself for ETH and sells it back to the market. Since this is done in immediate succession it should not be difficult for him to buy and resell it with next to zero loss, and setting up the lien with himself as the borrower is just a circular transfer of funds.
He now has a lien with the same tokenId as the original lender. This means that as soon as someone returns the NFT to the original lender, the attacker can withdrawNftWithInterest() and obtain the NFT.
In withdrawNftWithInterest() check explicitly that the NFT is not on loan:
if (lien.loanStartTime != 0) { revert Errors.LoanStarted(); }
Invalid Validation
#0 - c4-judge
2023-06-06T09:29:45Z
hansfriese marked the issue as duplicate of #13
#1 - c4-judge
2023-06-06T09:29:54Z
hansfriese marked the issue as satisfactory
#2 - c4-sponsor
2023-06-07T01:00:57Z
wukong-particle marked the issue as sponsor confirmed
#3 - wukong-particle
2023-06-07T01:00:59Z
Judge is correct, indeed duplication.
#4 - wukong-particle
2023-06-12T22:48:15Z
Data not available
A lienee whose NFT is not currently on loan may be prevented from withdrawing it.
A lienee who wishes to withdraw his NFT calls withdrawNftWithInterest() which tries to IERC721.safeTransferFrom() the NFT, which therefore reverts if the NFT is not in the contract (being on loan).
A griefer might therefore sandwich his call to withdrawNftWithInterest() with a swapWithEth() and a repayWithNft().
swapWithEth() removes the NFT from the contract, which causes the following withdrawNftWithInterest() to revert. For this the griefer has to pay lien.credit + lien.price. But this is returned in full in repayWithNft() minus payableInterest which is nothing since the loan time is zero.
Allow the option to, at any time, set a lien to not accept a new loan.
DoS
#0 - hansfriese
2023-06-06T07:47:09Z
No economical benefit for the attacker.
#1 - c4-judge
2023-06-06T07:47:15Z
hansfriese marked the issue as unsatisfactory: Invalid
#2 - wukong-particle
2023-06-07T00:46:40Z
Judge is correct. And this NFT swap behavior can happen any time (similar https://github.com/code-423n4/2023-05-particle-findings/issues/19), there's no particular benefit to do so as a sandwich attack.
#3 - c4-sponsor
2023-06-07T00:47:01Z
wukong-particle marked the issue as sponsor disputed
#4 - d3e4
2023-06-07T11:38:22Z
No economical benefit for the attacker.
There doesn't have to be. This is a pure griefing attack to prevent the lienee from withdrawing, hence rated only Medium.
#5 - hansfriese
2023-06-07T13:51:55Z
@wukong-particle I think this grief attack does not incur a direct loss for the lender but if the NFT ownership could yield any other type of profit, this can lead to an implicit economical loss for the lender. From this viewpoint, I am leaning to agree with the MEDIMUM severity although the sandwich attack costs significantly on the main net.
#6 - c4-judge
2023-06-07T13:52:05Z
hansfriese marked the issue as satisfactory
#7 - wukong-particle
2023-06-07T15:48:41Z
@hansfriese I agree this is a pure grief attack with no economic incentive. This could be MED or LOW severity issue because grief can technically raid any swap feature (even for any protocol). In the report please point out the pure grief nature of this attack -- we will consider patching this (e.g., minimum fee for opening a position), but if we leave this grief attack we want to acknowledge that there's no loss or anything associated with it. Thanks!
#8 - c4-judge
2023-06-07T16:17:22Z
hansfriese marked the issue as selected for report
#9 - romeroadrian
2023-06-07T16:26:36Z
@hansfriese I addressed this in L-9 here https://github.com/code-423n4/2023-05-particle-findings/blob/main/data/adriro-Q.md
There's no incentive here other than the grief, and the attacker will only have to pay gas. I believe the report has overinflated severity and should be downgraded to low. The recommendation also doesn't make sense cause the option could also be sandwiched by the same attack. Tagging the sponsor to hear their thoughts @wukong-particle
#10 - d3e4
2023-06-07T16:46:14Z
@hansfriese I addressed this in L-9 here https://github.com/code-423n4/2023-05-particle-findings/blob/main/data/adriro-Q.md
There's no incentive here other than the grief, and the attacker will only have to pay gas. I believe the report has overinflated severity and should be downgraded to low. The recommendation also doesn't make sense cause the option could also be sandwiched by the same attack. Tagging the sponsor to hear their thoughts @wukong-particle
I completely agree L-9 in #28 is a duplicate.
But what is this idea that there must be an economic incentive? Has there been a change of severity categorisation that I am not aware of? Medium is explicitly meant for when the assets are not at direct risk but for example when availability is impacted. Grief is a very standard Medium attack. The attacker might do it just out of pure spite. The point is to protect users, not to prevent attackers from profiting.
The recommendation cannot be attacked in the same way. If the user sets the lien to not accept any new loan, then as soon as the attacker repays the loan he cannot retake it again, and then the lender can withdraw it.
#11 - hansfriese
2023-06-07T18:18:54Z
@d3e4 I would invite you to provide precedents that can support your opinion. The economic benefit affects the likelihood of the attack.
But what is this idea that there must be an economic incentive? Has there been a change of severity categorisation that I am not aware of? Medium is explicitly meant for when the assets are not at direct risk but for example when availability is impacted. Grief is a very standard Medium attack. The attacker might do it just out of pure spite. The point is to protect users, not to prevent attackers from profiting.
#12 - hansfriese
2023-06-07T18:35:13Z
@romeroadrian I missed it, will upgrade L-9.
#13 - romeroadrian
2023-06-07T18:41:54Z
But what is this idea that there must be an economic incentive? Has there been a change of severity categorisation that I am not aware of? Medium is explicitly meant for when the assets are not at direct risk but for example when availability is impacted. Grief is a very standard Medium attack. The attacker might do it just out of pure spite. The point is to protect users, not to prevent attackers from profiting.
This doesn't make sense at all. A griefer can also spam the network to fill blocks dosing every protocol in the chain. I think you are consistently trying to inflate the severity of your reports using very vague arguments.
#14 - c4-judge
2023-06-08T08:16:40Z
hansfriese marked the issue as primary issue
#15 - wukong-particle
2023-06-09T18:59:35Z
We decided leave this grief in this round of contract updating and we will come back to it if there's really large volume of grief happening. We just wanted to emphasize the pure grief nature of this shenanigan since there's no loss or anything associated with it. Thanks!
#16 - d3e4
2023-06-10T13:34:51Z
@d3e4 I would invite you to provide precedents that can support your opinion. The economic benefit affects the likelihood of the attack.
But what is this idea that there must be an economic incentive? Has there been a change of severity categorisation that I am not aware of? Medium is explicitly meant for when the assets are not at direct risk but for example when availability is impacted. Grief is a very standard Medium attack. The attacker might do it just out of pure spite. The point is to protect users, not to prevent attackers from profiting.
These seem to be griefing with no economic benefit for the attacker: https://github.com/code-423n4/2023-02-ethos-findings/issues/381 https://github.com/code-423n4/2023-01-reserve-findings/issues/384 https://github.com/code-423n4/2023-01-astaria-findings/issues/324 https://github.com/code-423n4/2022-09-nouns-builder-findings/issues/182
I think the main argument would be that if a direct griefing is possible and there is an economic gain for the attacker, this would immediately become High severity as it is a direct compromise on assets. It would equally be High if the grief is permanent after a one-time attack, which is a definitive loss of assets, even though they don't end up with the attacker. In this case the assets are not permanently lost so it doesn't quite reach that High level, but it is the closest step below, and a direct impact on function and availability.
🌟 Selected for report: bin2chen
Also found by: d3e4, minhquanym
Data not available
The lender can prevent the borrower of his NFT from returning it, forcing him to pay interest for longer.
The borrower returns the NFT owed by calling repayWithNft(lien, lienId, tokenId). This will then validateLien(lien, lienId) which checks that the lien is hashed to the same digest as is stored at liens[lienId].
The issue is that the lender can frontrun the repayWithNft() with a call to startLoanAuction() and then call stopLoanAuction(). startLoanAuction() updates the lien with a new auctionStartTime. This changes the liens[lineId] which causes the validateLien() in the call to repayWithNft() to revert. The lender then stops the auction (and there's no rush really since no one is expected to sell an NFT to the contract for the very cheap early auction price) to return to business as usual. liens[lineId] is now also back to normal and the process would have to repeat.
The same procedure can be done with addCredit(), which can be called by anyone, instead of startLoanAuction(). Only 1 wei needs to be spent.
In this case liens[lineId] remains changed.
By persistently denying the borrower he will be forced to keep paying interest until his credit is consumed.
DoS
#0 - c4-judge
2023-06-06T10:27:27Z
hansfriese marked the issue as duplicate of #16
#1 - hansfriese
2023-06-06T10:31:41Z
Mitigation steps missing. Thus not chosen as a primary.
#2 - c4-sponsor
2023-06-07T00:52:21Z
wukong-particle marked the issue as sponsor confirmed
#3 - wukong-particle
2023-06-07T00:52:22Z
Judge is correct, indeed duplication.
#4 - c4-judge
2023-06-07T06:16:25Z
hansfriese marked the issue as satisfactory
#5 - c4-judge
2023-06-07T06:37:48Z
hansfriese changed the severity to 2 (Med Risk)
#6 - d3e4
2023-06-07T16:05:43Z
I don't think this is a duplicate of #16.
#16 is about DoS-ing lien validation by changing its hash by calling addCredit(). The mitigations would be to restrict access to the borrower or increase the minimum amount of credit one can add. My #41 is already duped to that issue, which I can agree with. What I said here about addCredit() was just a bonus and not the main issue.
The issue here in #40 is that lien validation can be broken by starting an auction. This is an entirely different lien validation DoS and cannot be mitigated by the mitigation of #16. The fact that the mitigations must be entirely different I think is evidence enough that they are separate issues. This is also why I didn't add a suggested mitigation here, because I couldn't think of one, even though I clearly had one for #16 in my #41.
To justify the High rating, note that the exploit here is to prevent the borrower from repaying his loan. This directly forces him to pay more interest. It is especially exploitable when the borrower is close to becoming insolvent. If the borrower is prevented from repaying so that he becomes insolvent, the lender can profitably liquidate him (the borrower can be DoS'd from adding credit to remain solvent separately if needed).
#7 - wukong-particle
2023-06-07T23:14:23Z
I see, I think the flow is logically correct. I misread this issue with the judge earlier. But this doesn't look like falling outside the designed behavior. Note that lender can still execute buy during auction.
About mitigation, we can potentially force an opened auction to remain open for, say, 1 hour. Then lender can simply execute again under attack (borrower attack at most once every hour). Does this mitigation look reasonable to you @d3e4
The 1 hour restriction is added with https://github.com/Particle-Platforms/particle-exchange-protocol/pull/28
#8 - d3e4
2023-06-08T13:51:29Z
I see, I think the flow is logically correct. I misread this issue with the judge earlier. But this doesn't look like falling outside the designed behaviour. Note that lender can still execute buy during auction.
The point of the issue is to cause the borrower's lien validation to fail by changing the lien data just in front of his call. If the borrower wants to try again by executing a buy during the auction, now with the new lien with an auctionStartTime, the lender can again DoS him by stopping the auction, if he hadn't already. The lender doesn't care about the auction running or not, only about changing the lien data.
About mitigation, we can potentially force an opened auction to remain open for, say, 1 hour. Then lender can simply execute again under attack (borrower attack at most once every hour). Does this mitigation look reasonable to you @d3e4
"Then lender can simply execute again under attack (borrower attack at most once every hour)." should swap "lender" and "borrower". But yes, this mitigation would prevent the lender from repeating the DoS on the borrower if he tries again within an hour. This makes the attack unviable and provides a guaranteed backup attempt for the borrower against this DoS. It also seems to make sense that all legitimate auctions would run for at least some time. A potential concern might be that a lender who starts an auction by mistake cannot immediately stop it, but since the price starts from zero there is plenty of margin already taken into account, so the first hour of the auction is effectively just a prelude during which no one is going to take the offer. The lender could in principle have set a lien price so high that the auction becomes interesting even within the first hour, but in this case he would probably not have found a borrower in the first place.
#9 - hansfriese
2023-06-12T06:53:07Z
Please note that there were many DoS-related reports and I just grouped them into two.
validateLien to failAs a side note, I would like to mention that this report lack Recommended Mitigation steps that can lead to invalidation.
Per C4 documentation:
The validity of an audit report submission is not based on whether it is ‘true’ or not. A report may contain a finding which is factually 'true' (the most literal interpretation of 'valid'), but if it does not add value or if it is not presented in such a way that adds value to a sponsor, it may be deemed invalid by a judge. This may seem harsh and exclusive, but it is essential to consider that Code4rena runs audit contests, not gotcha-hunts, and Code4rena offers guaranteed payout for valid submissions. This means that wardens are providing a service to sponsors and the product of those services should meet what judges feel is a minimum standard in order to be deemed of value.
#10 - d3e4
2023-06-12T10:02:14Z
Please note that there were many DoS-related reports and I just grouped them into two.
I can see your reasoning for grouping them based on changing the lien data. But it is important to note that changing the lien data is not an issue in itself and therefore cannot be the basis for grouping issues. It would be like saying that having funds in the contract is an issue and therefore group all issues which steal funds. The fact that changing the lien data will DoS lien validation is not going to change; what needs to be done is to make sure that this cannot be exploited. It can be exploited in several distinct ways, just like funds might be stolen in many ways. #16 and #40 are similar only in their connection to lien validation; the exposed vulnerabilities are entirely different. Please note that #16 doesn't describe a High severity exploit, but just the mechanism. #8 and #41 do however describe how this can be fully exploited. Unless the lien storage implementation is completely overhauled, fixing {#16; #8, #41} cannot fix #40; they are different ways of exploiting how liens are stored.
Even if you consider {#16; #8, #41} and #40 to be duplicates it is still a fact that #40 and #41 together are much broader in terms of root issues described and demonstrated impact than #16.
As a side note, I would like to mention that this report lack Recommended Mitigation steps that can lead to invalidation.
Revealing an issue is surely more critical than coming up with a mitigation? Would you not agree that a lack of recommended mitigation is better than a faulty one? sockdrawer and Trust think so. But note in the discussion with the sponsor above that this report did indeed lead to a mitigation (different from #16).
#11 - hansfriese
2023-06-12T10:38:43Z
I can see your reasoning for grouping them based on changing the lien data. But it is important to note that changing the lien data is not an issue in itself and therefore cannot be the basis for grouping issues. It would be like saying that having funds in the contract is an issue and therefore group all issues which steal funds.
It sounds good you see the reasoning for grouping these issues although it's a pity your response is not respectful with an inappropriate generalization.
The fact that changing the lien data will DoS lien validation is not going to change; what needs to be done is to make sure that this cannot be exploited. It can be exploited in several distinct ways, just like funds might be stolen in many ways. https://github.com/code-423n4/2023-05-particle-findings/issues/16 and https://github.com/code-423n4/2023-05-particle-findings/issues/40 are similar only in their connection to lien validation; the exposed vulnerabilities are entirely different. Please note that https://github.com/code-423n4/2023-05-particle-findings/issues/16 doesn't describe a High severity exploit, but just the mechanism. https://github.com/code-423n4/2023-05-particle-findings/issues/8 and https://github.com/code-423n4/2023-05-particle-findings/issues/41 do however describe how this can be fully exploited. Unless the lien storage implementation is completely overhauled, fixing {https://github.com/code-423n4/2023-05-particle-findings/issues/16; https://github.com/code-423n4/2023-05-particle-findings/issues/8, https://github.com/code-423n4/2023-05-particle-findings/issues/41} cannot fix https://github.com/code-423n4/2023-05-particle-findings/issues/40; they are different ways of exploiting how liens are stored.
Sorry, I can't find new information here.
Even if you consider {https://github.com/code-423n4/2023-05-particle-findings/issues/16; https://github.com/code-423n4/2023-05-particle-findings/issues/8, https://github.com/code-423n4/2023-05-particle-findings/issues/41} and https://github.com/code-423n4/2023-05-particle-findings/issues/40 to be duplicates it is still a fact that https://github.com/code-423n4/2023-05-particle-findings/issues/40 and https://github.com/code-423n4/2023-05-particle-findings/issues/41 together are much broader in terms of root issues described and demonstrated impact than https://github.com/code-423n4/2023-05-particle-findings/issues/16.
What's the point of your statement "grouping two issues is much broader"? Do you mean you want your two findings including all your comments to be judged as a single report?
Revealing an issue is surely more critical than coming up with a mitigation? Would you not agree that a lack of recommended mitigation is better than a faulty one? sockdrawer and Trust think so. But note in the discussion with the sponsor above that this report did indeed lead to a mitigation (different from https://github.com/code-423n4/2023-05-particle-findings/issues/16).
Being included in the mitigation does not justify your report should be awarded. And I would like to remind you I am the judge for this contest.
Please remember the guideline.
At the point that a judge provides a response, the conversation is over, unless you wish to provide additional facts which demonstrate inaccurate assumptions in the judge's response. Further pursuit of the conversation will result in having backstage access suspended.
#12 - d3e4
2023-06-12T11:00:04Z
It sounds good you see the reasoning for grouping these issues although it's a pity your response is not respectful with an inappropriate generalization.
I apologise, I did not mean to be disrespectful. I just used this as an extreme example to illustrate my point, the point of contact being something which is an accepted part of the protocol.
What's the point of your statement "grouping two issues is much broader"? Do you mean you want your two findings including all your comments to be judged as a single report?
Not my comments. My two issues #40 and #41, if considered one report (since you consider them duplicates), includes more vulnerabilities, exploits and impact than #16 and #8. #16 and #8 are a subset of what is reported in #40 and #41. I argued that #40 and #41 should be considered two separate issues, but if you think they are too similar and should be awarded as one issue then that's a judgement I cannot dispute. But in that case #16 and #8 should be considered partials of the #40/#41 issue, since they are but subsets.
Please remember the guideline.
At the point that a judge provides a response, the conversation is over, unless you wish to provide additional facts which demonstrate inaccurate assumptions in the judge's response. Further pursuit of the conversation will result in having backstage access suspended.
This is indeed about inaccurate assumptions.
🌟 Selected for report: bin2chen
Also found by: d3e4, minhquanym
Data not available
An auction can be forced to conclude, which is typically to the benefit of the lender.
auctionBuyNft(lien, lienId, tokenId, amount) validates the lien in question by validateLien(lien, lienId) which checks that the lien is hashed to the same digest as is stored at liens[lienId].
The issue is that the attacker (such as the lender holding the auction) can frontrun auctionBuyNft() with a call to addCredit() (spending only 1 wei) which updates the lien and its stored digest. This causes the auctionBuyNft() to revert in its validateLien().
Frontrunning every attempt to have the contract buy the NFT the lender can thus force the auction to conclude. He can then liquidate the borrower for his desired price. He can initiate this attack as soon as the NFT is borrowed, because typically the desired price would be higher than the borrower expects to sell it for on the market. This implies an quick (24 hours) gain for the lender at the cost of the unsuspecting borrower.
A solution might be to require that addCredit() makes the borrower solvent as per the current auction price.
DoS
#0 - c4-judge
2023-06-06T09:41:54Z
hansfriese marked the issue as duplicate of #31
#1 - c4-judge
2023-06-06T09:42:35Z
hansfriese marked the issue as not a duplicate
#2 - c4-judge
2023-06-06T09:42:45Z
hansfriese marked the issue as satisfactory
#3 - c4-judge
2023-06-06T09:51:22Z
hansfriese marked the issue as duplicate of #8
#4 - c4-judge
2023-06-06T10:37:35Z
hansfriese marked the issue as duplicate of #16
#5 - wukong-particle
2023-06-07T00:50:54Z
Duplication of #8 and #16 is correct, not a duplicate to #31 though.
#6 - c4-sponsor
2023-06-07T00:50:57Z
wukong-particle marked the issue as sponsor acknowledged
#7 - c4-sponsor
2023-06-07T00:51:52Z
wukong-particle marked the issue as sponsor confirmed
#8 - c4-judge
2023-06-07T06:37:48Z
hansfriese changed the severity to 2 (Med Risk)
#9 - wukong-particle
2023-06-12T22:43:26Z
🌟 Selected for report: minhquanym
Data not available
The protocol can steal all interest accrued to lenders, and cannot guarantee that the lenders will receive their due interest.
_treasuryRate determines the proportion of the interest paid to the protocol whenever interest is paid, which happens in _withdrawAccountInterest():
function _withdrawAccountInterest(address payable lender) internal { uint256 interest = interestAccrued[lender]; if (interest == 0) return; interestAccrued[lender] = 0; if (_treasuryRate > 0) { uint256 treasuryInterest = MathUtils.calculateTreasuryProportion(interest, _treasuryRate); _treasury += treasuryInterest; interest -= treasuryInterest; } lender.transfer(interest); emit WithdrawAccountInterest(lender, interest); }
_treasuryRate can be set freely, from 0% to 100%, by the owner:
function setTreasuryRate(uint256 rate) external onlyOwner { if (rate > MathUtils._BASIS_POINTS) { revert Errors.InvalidParameters(); } _treasuryRate = rate; emit UpdateTreasuryRate(rate); }
This means that the owner can frontrun each call to _withdrawAccountInterest() (through the external calls to withdrawNftWithInterest(), withdrawEthWithInterest() or withdrawAccountInterest()) by a setTreasuryRate(_BASIS_POINTS) (100%) and steal all the interest accrued to the lender.
Thus the protocol cannot offer any guarantee to the lender that he will be given his due interest.
Decide on a fixed _treasuryRate, or a maximum value which lenders are always willing to accept, or implement a time delay so that lenders can react to the change of terms.
Rug-Pull
#0 - c4-judge
2023-06-06T06:24:08Z
hansfriese marked the issue as duplicate of #9
#1 - c4-judge
2023-06-06T06:29:28Z
hansfriese marked the issue as unsatisfactory: Overinflated severity
#2 - c4-judge
2023-06-06T06:30:03Z
hansfriese changed the severity to 2 (Med Risk)
#3 - c4-judge
2023-06-06T07:53:05Z
hansfriese marked the issue as satisfactory
#4 - wukong-particle
2023-06-07T00:59:33Z
Judge is correct, indeed duplication
#5 - c4-sponsor
2023-06-07T00:59:37Z
wukong-particle marked the issue as sponsor confirmed
#6 - romeroadrian
2023-06-07T16:32:20Z
@hansfriese reported this in L-2 https://github.com/code-423n4/2023-05-particle-findings/blob/main/data/adriro-Q.md#l-2-_withdrawaccountinterest-can-be-front-runned-to-increase-the-treasury-rate
Again I believe the severity is severely inflated as it requires owner connivance, thus centralization risk. Note that admin also have the possibility of upgrading the contract and basically do anything.
#7 - wukong-particle
2023-06-07T22:59:51Z
I agree with adrian, this could be low severity issue. We will likely add an upper bound for the treasury rate in our fix.
#8 - wukong-particle
2023-06-12T22:47:44Z
Mitigated with https://github.com/Particle-Platforms/particle-exchange-protocol/pull/12 (imposed a max treasury rate)
🌟 Selected for report: adriro
Also found by: bin2chen, d3e4, minhquanym, rbserver
Data not available
Since the call to stopLoanAuction() requires the lien to be validated, any change in the lien will revert the call. A frontrunner might change the stored lien digest just before the lender's call, which causes the lien validation to revert, by calling addCredit() (with any non-zero amount).
This might be mitigated by requiring that addCredit() makes the borrower solvent as per the current auction price.
MAX_PRICE is set to only 1000 ETH. This seems to be too restrictive considering what exorbitant sums some NFTs are sold for.
There are rounding errors of the form a*(b/c) <= a*b/c here
interest = principal.wadMul(bipsToWads(rateBips).wadMul(loanTimeWad.wadDiv(_YEAR_WAD)));
and here
return price.wadMul(auctionElapsed.wadDiv(auctionDuration));
Consider dividing the entire product last.
Using a proprietary WETH implementation might instil doubt in some users. Consider using the already deployed and accepted WETH9 at 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2.
receive only accepts from approved marketThe only time ParticleExchange receives funds through receive() is from a market place when selling an NFT via sellNftToMarket(), which only allows registered marketplaces. Consider similarly protecting receive() lest funds be elsewhence sent and lost.
buyNftFromMarket() should not be payableThe trader is not supposed to send any funds in the call to buyNftFromMarket(); the funds are taken from the remaining credit and price which are required to cover the buy amount. Any funds sent in this call will be lost.
Consider changing buyNftFromMarket() to nonpayable (in IParticleExchange.sol as well).
It is stated in the gitbook as well as in the comments that the auction price increases linearly. This is not quite correct. The auction price increases linearly with time AND max price, but since the max price also increases linearly with time as interest accrues the auction price actually increases quadratically. This might be worth a note.
interst -> interest accured -> accrued linearly increase in time -> increases linearly with time accured -> accrued accured -> accrued accured -> accrued accure -> accrue accure -> accrue accure -> accrue accure -> accrue If procceds -> If it proceeds
#0 - c4-judge
2023-06-06T10:47:11Z
hansfriese marked the issue as grade-b
#1 - wukong-particle
2023-06-06T23:29:14Z
Acknowledged, similar to https://github.com/code-423n4/2023-05-particle-findings/issues/16
1000 ETH would be the floor of this NFT collection. Will restrict to this large number for now.
Acknowledged.
Acknowledged the mul, div order, will consider.
We are using WETH at contract initialization time, and it's publicly verifiable. Only reason not hardcoding this in contract is that we need to unit test WETH with forge and we can't use 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2.
Acknowledged, not checking to save gas. We are not liable if arbitrary sender just sends the contract fund.
Acknowledged, similar to https://github.com/code-423n4/2023-05-particle-findings/issues/23
Acknowledged, the interest accrued during auction should be quite small, our frontend will show linearity for easier understanding, contract will impose https://github.com/code-423n4/2023-05-particle-findings/blob/main/data/adriro-Q.md#L-6
Acknowledged the typos, will update
#2 - c4-sponsor
2023-06-06T23:30:29Z
wukong-particle marked the issue as sponsor acknowledged
#3 - hansfriese
2023-06-07T12:39:52Z
1 - Nullified with a similar finding from the same warden 2 - L 3 - L 4 - L 5 - L 6 - L 7 - L 8 - L 9 - N
L7 N1
#4 - hansfriese
2023-06-07T12:51:33Z
#45 downgraded to LOW.
L8 N1
#5 - wukong-particle
2023-06-09T23:32:24Z
1 mitigated with https://github.com/Particle-Platforms/particle-exchange-protocol/pull/14 3, 4 fixed (nullified) with https://github.com/Particle-Platforms/particle-exchange-protocol/pull/11 7 fixed with https://github.com/Particle-Platforms/particle-exchange-protocol/pull/7 9 fixed with https://github.com/Particle-Platforms/particle-exchange-protocol/pull/25
🌟 Selected for report: adriro
Also found by: d3e4, minhquanym
Data not available
- return (bips * 1e18) / _BASIS_POINTS; + return bips * 1e14;
Consider using the already deployed and accepted WETH9 at 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2 to save the gas from deploying your own.
safeTransferFrom() can be transferFromIn supplyNft() the NFT is transferred to self which we know can receive an ERC721, so there is no need to use safe transfer, which wastes gas by also calling onERC721Received().
#0 - c4-judge
2023-06-06T10:45:11Z
hansfriese marked the issue as grade-b
#1 - wukong-particle
2023-06-06T23:44:54Z
Acknowledged, not simplified for readability and maintainability
We are using WETH9 at deployment. Not hardcoding it in contract only because foundry unit tests can't use this fixed address.
Acknowledged, will update, similar to the 7th point in https://github.com/code-423n4/2023-05-particle-findings/blob/main/data/adriro-G.md#particleexchange-contract
#2 - c4-sponsor
2023-06-06T23:45:12Z
wukong-particle marked the issue as sponsor acknowledged
#3 - wukong-particle
2023-06-09T23:34:35Z