-
Notifications
You must be signed in to change notification settings - Fork 1
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
KupiaSec - The VaultLib.__liquidateUnchecked()
function unnecessarily reorders the already correctly ordered values of raReceived
and ctReceived
from UniswapV2Router
#214
Comments
yes this is valid and have been fixed prior to our trading competition. will provide links later |
VaultLib.__liquidateUnchecked()
function unnecessarily reorders the already correctly ordered values of raReceived
and ctReceived
from UniswapV2Router
VaultLib.__liquidateUnchecked()
function unnecessarily reorders the already correctly ordered values of raReceived
and ctReceived
from UniswapV2Router
You've deleted an escalation for this issue. |
@AtanasDimulski, I think you misunderstood something. The issue is simply due to the unnecessary application of the That function reorders the amounts, which were already well-ordered. |
@KupiaSecAdmin you are correct, my mistake. Nice catch! |
escalate This issue is invalid because it requires the ra address to be bigger than the ct address.
|
You've created a valid escalation! To remove the escalation from consideration: Delete your comment. You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final. |
I don't agree with your insistence.
|
@KupiaSecAdmin still, the admin picks the ra and ct address, so according to the rule, it is trusted to pick values that do not cause any issues. |
I totally disagree with your insistence.
When issuing a new This is a clear bug, and the sponsor has also confirmed it. So, I believe the judge will make the correct decision. |
It does not have to cancel it, it can create dummy ra, ct pairs. Additionally, the admin can order the issuance of ra, ct pairs according to their addresses. If we have ra1 = 5, ra2 = 3, ct1 = 4, ct2 = 6, the admin can do pairs ra2,ct1 and ra1,ct2. If the ra address is bigger than the next ct, and the admin really wants to use this ra, he can create other pairs to increase the ct address and then create the pair with the correct ct. Another option for the admin is to create a wrapper for ra that has a smaller address, if needed. I am not saying the bug does not exist, it does, but it is invalid according to the sherlock rules, as it is picked solely by the admin and the admin is trusted to use values that do not cause issues. |
The protocol team fixed this issue in the following PRs/commits: |
Your assertions do not align with the Sherlock rule you mentioned. 1. The method by which the admin controls
|
The admin is trusted to use the right ra and ct values, and they are fully in their control. There are ra and ct values that the admin can use that do not cause any issues. Hence, the rule applies. The id in the function points to a specific ra, so it can choose the ra value. The ct value comes from the nonce, as explained earlier, and can also be picked to not cause any issues. |
CT is not under the admin's control; it is clearly an internal address that the admin disregards.
For a given Therefore, the rule does not apply. I believe there may have been a misunderstanding. Once again, I trust that the judge will make the right decision. |
The admin would disregard it if it was not trusted. But he is trusted, so he should do the correct thing and use proper ra and ct values, which is possible, as mentioned before. As per the argument that the admin should not create dummy ct addresses to get one higher than ra and prevent this issue, this is the reason #180 was invalidated, as the admin could do this to create another pool. So here the admin must also be able to use a proper ct and mitigate this issue. |
As I understand, regardless of the admin input, the |
@WangSecurity if ra is token0 the issue does not happen. As the admin picks these addresses, it is trusted to not cause issues. And the ra and ct addresses are part of the admin input, the |
I have to agree with @0xsimao here , since the admin can prevent the issue by picking the correct address it should not be an issue |
We are focusing on a specific fixed As I mentioned earlier:
Therefore, the Sherlock rule does not apply to my issue. Additionally, the protocol's implementation of mechanisms to sort reserve values in the Uniswap V2 pool at multiple points indicates that the admin does not care |
I would agree about the admin rule if the admin indeed could just specify ra and ct they want to use, but here, for that to happen, the admin would have to do additional manipulations that are not relevant to creating a pair (e.g. create dummy ra, ct pairs). Moreover, regardless of the admin input in Hence, the decision remains, reject the escalation and leave the issue as it is. |
I do not understand this part, if the admin picks ra and ct correctly the issue does not happen. |
As was said above, the admin doesn't just pick ra and ct, and for them to be ordered without any issues after the second re-order, the admin has to complete manipulations irrelevant to creating a new pair. Regardless of the admin's input, they will still be ordered correctly in The decision remains, reject the escalation and leave the issue as it is. |
When will it work? If a nonce increment of 100 is required, can the admin afford such a significant gas cost? Even, for every new issuance? What if a nonce increment of 1000 is necessary? Impractical!
No. If all other I prefer not to continue discussing the uncertain actions you’re proposing, as they lack guarantees and practicality. Additionally, the Sherlock rule does not state that the admin can use undesigned function variables (your dummy |
They may use dummy cts if the other ids are not enough. The rule applies because not all ras cause issues. Even ra addresses that cause issues can be fixed by using dummy ct addresses. And if they deem it impractical they can redeploy without losses. And the admin is trusted to use ra addresses that are practical for them (not being very big in this case). But the issue present in this report will not happen because the admin is trusted to use ra values that do not cause issues. |
Still it has no guarantee.
Redeploy? There’s still no guarantee that a similar situation won’t occur with the new deployment. Additionally, redeploying is not the approach the Sherlock rule recommends for preventing issues. Let’s not extend your uncertain actions into even more unguaranteed and impractical solutions. |
I do not know what they would do if they redeployed but according to the admin rule the issue described in this report will not happen for sure. |
There’s no logical context here. |
The context is that ra < ct does not cause issues |
Please don’t repeat the same logic which is not perfect. |
Actually, I'll revert to my previous decision to leave the issue as is. The admin indeed supplies the RA address, while CT is not an admin input. Yes, there are some theoretical ways for the admin to manipulate the CT address, but @KupiaSecAdmin very well explained why these ways are impractical and are not how the protocol is designed to work. Moreover, this can happen with fairly any RA address besides very small addresses, which are not even tokens. Hence, I agree that admin rules here are not applicable because the way to potentially mitigate an issue are impractical and are not expected. Planning to reject the escalation and leave the issue as it is, will apply that decision tomorrow 10 AM UTC. |
Hi @WangSecurity However, the issue #174 also mentioned a fix where the admin had to donate in the uniV2 pool and call sync in order to mitigate for all DS issuances. Moreover, the reason of invalidating #114 is also admin intervention , it is not justified that for some issues the admin rule is stretched or loosely applied just because the admin intervention is higher , all such issues where admin can somehow fix the issue should be invalidated or it does not make sense to invalidate some since anyhow admin has to go the extra mile and fix regardless if its tough or easy. |
There is a fundamental difference between #174 and both #214 and #114. In #174 the problem is introduced by a malicious actor, in #214 and #114 the code doesn't work as expected. However, I agree that #214 and #114 should be either both valid, or both invalid. I strongly belive both of those issues are valid. |
@AtanasDimulski |
@Pheonix244001 this is not correct, your issue assumes that pairs will be deployed with 0 liquidity, and you haven't provided any pre conditions as to how this can happen. Then a malicious actor provides only 1 RA in an already deployed pair, and this isn't even a DOS because the next block someone donates 1 CT and the issue is resolved, the attacker can't do anything to continue the DOS. Both #214 and #114 originate from the fact that the code doesn't work as expected without a malicious actor performing any actions to cause the issue. Then the actions that the admin has to execute are way more complicated, those types of actions are impractical and not expected from an admin. |
@AtanasDimulski please read the original comment , the fact that admin has to intervene and perform actions himself (as mentioned in the report for all new DS issuances ) is sufficient proof itself that all admin interventions leading to fixing the issue should be counted and hence all of the issues should be invalidated or validated , loosening the rules and definitions based on how hard/easy is the action is not justified.
This is not an assumption see issueNewDs() where a new pair is created. |
Firstly, #174 wasn't invalidated based on the admin rules and I don't think I mentioned them there (even if mentioned, the decision wasn't based on them in the end). The decision remains the same, reject the escalation and leave the issue as it is. Planning to apply the decision in a couple of hours. |
Let me reiterate #174 here for one last time here: The same can be applied after expiry when issueNewDs() is called again. In both the cases, we require admin intervention to manually donate ct to the pair in order for addLiquidity to work again. There can be hundreds of pairs for which issueNewDs() is called. |
@WangSecurity I have to agree on the fact that "how hard" it is for the admin to mitigate the issue should not be a roadblock and decisive if one issue is validated and the other is not , the fact that both issues can be mitigated by admin acctions should be treated equally , admin rules have to be same for all regardless |
I have read the function numerous times as I spend a lot of time defending #114, you are omitting the part that if in the previous pair there is liquidity that liquidity will be provided to the newly created pair, and the whole scenario you described becomes obsolete. The pre-conditions you haven't provided are that the pair has to be created with 0 liquidity and how this can happen. |
@WangSecurity you can't say that because of the fact that in #214 the admin may have to deploy hundreds of pairs, this is not practical. But assume in #114 that the admin has to donate and manipulate the price of the previous uniV2 pair, is practical and completely disregard the fact that the only way he can achieve this in 100% of the cases is if he frontruns other users. Frontruing and saying that if someone else manipulates the price back falls under the 1 block DOS rules is exactly the same thing in this scenario. |
@AtanasDimulski and @SakshamGuruji3 made a good point. I see your side and how it would be unfair to validate one finding but invalidate another one in this case. To keep the fairness and consistency, I will change my decision now again towards invalidating this. Tagging @KupiaSecAdmin for transparency. The decision to accept and invalidate will be applied in a couple of hours.
|
Result: |
Escalations have been resolved successfully! Escalation status:
|
I have a question about the Sherlock rule.
Does this rule imply that the admin can use undesigned function variables (dummy I believe the rule indicates that any issues can be invalidated if the admin can prevent them by using and modifying legitimate function variables. Since dummy I have always stated, firstly, that the rule does not apply to this issue. Practicality is merely a secondary concern. |
I wouldn’t say this exact rule is applied here. The issue is invalidated, because it’s in the admin’s control to prevent the issue and if they can make calls in a way this issue doesn’t happen, this should be low/invalid |
KupiaSec
High
The
VaultLib.__liquidateUnchecked()
function unnecessarily reorders the already correctly ordered values ofraReceived
andctReceived
fromUniswapV2Router
Summary
The
__liquidateUnchecked()
function removes liquidity and receives the correspondingRA
andCT
from UniswapV2, returning the amounts ofRA
andCT
received. When removing liquidity, theremoveLiquidity()
function of theUniswapV2Router
returnsraReceived
andctReceived
, which are the exact amounts ofRA
andCT
corresponding to the removed liquidity, regardless of the order of tokensRA
andCT
in the Uniswap pool. However, in the__liquidateUnchecked()
function, these values are reordered as if they represent the received amounts oftoken0
andtoken1
, assuming thattoken0 = CT
andtoken1 = RA
.Vulnerability Detail
As noted in line 282 of the
__liquidateUnchecked()
function, theammRouter.removeLiquidity()
function returns two values:raReceived
andctReceived
. These values represent the exact amounts ofRA
andCT
received from the Uniswap pool, even in the case wheretoken0 = CT
andtoken1 = RA
(seeUniswapV2Router02.sol
). Therefore, there is no need to reorder these values. However, at line 284, the function reorders them, assumingtoken0 = CT
andtoken1 = RA
. As a result, whentoken0 = CT
andtoken1 = RA
, the values ofraReceived
andctReceived
will be interchanged. This leads to incorrect behavior in the _liquidatedLp() function, which processes expired states when issuing new DS. Consequently, an incorrectRA
amount is reserved in the reserve pool, potentially leading to a loss ofRA
for users.Impact
Loss of
RA
for users.Code Snippet
https://github.com/sherlock-audit/2024-08-cork-protocol/tree/main/Depeg-swap/contracts/libraries/VaultLib.sol#L270-L289
https://github.com/Uniswap/v2-periphery/blob/master/contracts/UniswapV2Router02.sol#L116
Tool used
Manual Review
Recommendation
Eliminate the reordering.
The text was updated successfully, but these errors were encountered: