Platform: Code4rena
Start Date: 18/11/2021
Pot Size: $50,000 USDC
Total HM: 18
Participants: 26
Period: 7 days
Judge: leastwood
Total Solo HM: 12
Id: 54
League: ETH
Rank: 8/26
Findings: 3
Award: $1,302.66
🌟 Selected for report: 4
🚀 Solo Findings: 2
🌟 Selected for report: itsmeSTYJ
itsmeSTYJ
Uniswap v2 made oracle attacks much more expensive to execute (since it needs to be manipulated over X number of blocks) however its biggest drawback is that it reacts slow to price volatility (depends on how far back you look). Depending on a single oracle is still very risky and can be exploited given the correct conditions.
Assuming the ideal conditions, it is possible to purchase many keys across many locks for the UDT token that is distributed to the referrer and sell them on some other exchanges where the price of UDT is higher; high enough such that the malicious user can still profit even after requesting for a refund (w/ or w/o a free trial).
This exploit is made possible because of:
The following assumptions has to be true for this attack to work:
udtOracle.updateAndConsult() only updates once per day, it is slow to react to the volatility of UDT price movements.#0 - julien51
2022-01-03T14:40:03Z
AS you noted this is pretty theoretical and given that the amount of UDT minted is capped to the gas spent, the user will need to 1) purchase a LOT of keys and 2) cancel them all and 3) find an exchange where the price is significantly different.
#1 - 0xleastwood
2022-01-12T01:23:20Z
Nice find!
While, I do agree this is a difficult attack to perform, it is still a valid way of extracting value from the protocol. Hence, I believe this should be kept as medium.
2 — Med (M): vulns have a risk of 2 and are considered “Medium” severity when assets are not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
#2 - julien51
2022-03-16T06:40:29Z
We will mitigate this in an upcoming upgrade by moving to Uniswap v3 for our oracles.
🌟 Selected for report: itsmeSTYJ
itsmeSTYJ
A malicious user is able to withdraw all payments that were paid to a lock owner if the owner increases the keyPrice.
When updateKeyPricing() is called to increase the price of a key, it is possible to frontrun this call and buy many keys at the cheaper price then request for a refund at the higher price.
Keep track of the price at which keys are purchased so that when you issue a refund, you use the original keyPrice to refund instead of the updated keyPrice
#0 - julien51
2022-01-03T14:32:14Z
This is only true for locks where there is no penalty. We should make it clear on the front-end that when changing the price it is recommended to set up a penalty (at least temporarily) for the price difference so that no key can be refunded for the full price.
#1 - 0xleastwood
2022-03-22T21:53:39Z
Circling back on this, I'm not sure how a penalty would be correctly applied to all locks. Wouldn't users who wanted to get a refund for their key get penalised if they purchase after the change in key price? I think it would also be safer to update the key price and apply the penalty in the one transaction.
#2 - julien51
2022-03-23T01:12:02Z
I think that is a good finding, but there again (like often) I think this is pretty edgy. The cancellation penalty is pretty easy to apply just to a single lock from the lock manager's perspective. Before changing the lock price, a lock manager can easily apply a penalty for the difference in price. IE if I change the price from 10 to 12, I apply a penalty for 2 and anyone who tries to abuse this will only get a refund of 12-2 = 10.
On top of that we're actually storing the amount paid for the latest key as part of our next upgrade to support automatically recurring memberships, which should make things even more robust as anyone will only get re-imbursed based on what they paid...
#3 - 0xleastwood
2022-03-23T01:34:04Z
Considering the sponsor's comments and after some further discussion on Discord. I think it is more correct to downgrade this to medium severity. While it isn't clear, the lock manager is expected to apply a penalty before updating the cost of a membership such that users cannot game the price difference. However, this isn't enforced on-chain or documented anywhere so based on the judge's and warden's context at the time, this seemed like a valid high severity issue. It is important to note that users who refund their membership after purchasing a membership post price change will be refunded less than users who purchased their memberships before the price change. The sponsor is looking to integrate these fixes in their next upgrade.
🌟 Selected for report: itsmeSTYJ
itsmeSTYJ
Gas op
In the _getCancelAndRefundValue() function, timeRemaining + freeTrialLength is used twice, you can save some gas by assigning it to a variable.
🌟 Selected for report: itsmeSTYJ
itsmeSTYJ
gas op
in the MixinPurchase._purchasePriceFor() function, you do not need this require statement require(unlockDiscount <= minKeyPrice, 'INVALID_DISCOUNT_FROM_UNLOCK'); because of unlockDiscount is > minKeyPrice, it will underflow when you try to subtract the former from the latter because the contract is compiled w/ solidity version ^0.8.0.