sherlock-admin3 commented 1 month ago

hash

Medium

Leftover dust debt can cause liquidation auction to occur at significantly lowered price

Summary

Using frob to refund the gem inside onRemove can disallow liquidations due to dust check

Vulnerability Detail

When an auction is removed (on completetion) from the clipper, the leftover amount of the auction if any, is refunded back to the user by calling the onRemove method

gh link


    function take(
        uint256 id,           // Auction id
        uint256 amt,          // Upper limit on amount of collateral to buy  [wad]
        uint256 max,          // Maximum acceptable price (DAI / collateral) [ray]
        address who,          // Receiver of collateral and external call address
        bytes calldata data   // Data to pass in external call; if length 0, no call is done
    ) external lock isStopped(3) {

        .....

        } else if (tab == 0) {
            uint256 tot = sales[id].tot;
            vat.slip(ilk, address(this), -int256(lot));
=>          engine.onRemove(usr, tot - lot, lot);
            _remove(id);
        } else {

After burning the associated fees, the remaining amount is credited to the urn by invoking vat.slip and vat.frob gh link

    function onRemove(address urn, uint256 sold, uint256 left) external auth {
        uint256 burn;
        uint256 refund;
        if (left > 0) {
            burn = _min(sold * fee / (WAD - fee), left);
            mkr.burn(address(this), burn);
            unchecked { refund = left - burn; }
            if (refund > 0) {
                // The following is ensured by the dog and clip but we still prefer to be explicit
                require(refund <= uint256(type(int256).max), "LockstakeEngine/overflow");
                vat.slip(ilk, urn, int256(refund));
=>              vat.frob(ilk, urn, urn, address(0), int256(refund), 0);
                lsmkr.mint(urn, refund);
            }
        }
        urnAuctions[urn]--;
        emit OnRemove(urn, sold, burn, refund);

But incase the urn's current debt is less than debt, the frob call will revert gh link

    function frob(bytes32 i, address u, address v, address w, int dink, int dart) external {

        ....

        require(either(urn.art == 0, tab >= ilk.dust), "Vat/dust");

This will cause the complete liquidation to not happen till there is no leftover amount which would occur at a significantly low price from the expected/market price. The condition of tab >= ilk.dust can occur due to an increase in the dust value of the ilk by the admin

Example: initial dust 10k, mat 1.5, user debt 20k, user collateral worth 30k liquidation of 10k debt happens and dust was increased to 11k now the 15k worth collateral will only be sold when there will be 0 leftover (since else the onRemove function will revert) assuming exit fee and liquidation penalty == 15% in case there was no issue, user would've got ~(15k - 11.5k liquidation penalty included - 2k exit fee == 1.5k) back, but here they will get 0 back so loss ~= 1.5k/30k ~= 5%

POC

Apply the following diff and run forge test --mt testHash_liquidationFail

diff --git a/lockstake/test/LockstakeEngine.t.sol b/lockstake/test/LockstakeEngine.t.sol
index 83fa75d..0bbb3fa 100644
--- a/lockstake/test/LockstakeEngine.t.sol
+++ b/lockstake/test/LockstakeEngine.t.sol
@@ -86,6 +86,7 @@ contract LockstakeEngineTest is DssTest {

     function setUp() public {
         vm.createSelectFork(vm.envString("ETH_RPC_URL"));
+        vm.rollFork(20407096);

         dss = MCD.loadFromChainlog(LOG);

@@ -999,6 +1000,66 @@ contract LockstakeEngineTest is DssTest {
         }
     }

+    function testHash_liquidationFail() public {
+        // config original dust == 9k, update dust == 11k, remaining hole == 30k, user debt == 40k
+        // liquidate 30k, 10k remaining, update dust to 11k, and increase the asset price/increase the ink of user ie. preventing further liquidation
+        // now the clipper auction cannot be fulfilled 
+        vm.startPrank(pauseProxy);
+
+        dss.vat.file(cfg.ilk, "dust", 9_000 * 10**45);
+        dss.dog.file(cfg.ilk, "hole", 30_000 * 10**45);
+
+        vm.stopPrank();
+        
+        address urn = engine.open(0);
+
+        // setting mkr price == 1 for ease
+        vm.store(address(pip), bytes32(uint256(1)), bytes32(uint256(1 * 10**18)));
+        // mkr price == 1 and mat = 3, so 40k borrow => 120k mkr
+        deal(address(mkr), address(this), 120_000 * 10**18, true);
+        mkr.approve(address(engine), 120_000 * 10**18);
+        engine.lock(urn, 120_000 * 10**18, 5);
+        engine.draw(urn, address(this), 40_000 * 10**18);
+
+        uint auctionId;
+       {
+        // liquidate 30k
+        vm.store(address(pip), bytes32(uint256(1)), bytes32(uint256(0.98 * 10**18))); // Force liquidation
+        dss.spotter.poke(ilk);
+        assertEq(clip.kicks(), 0);
+        assertEq(engine.urnAuctions(urn), 0);
+        auctionId=dss.dog.bark(ilk, address(urn), address(this));
+        assertEq(clip.kicks(), 1);
+        assertEq(engine.urnAuctions(urn), 1);
+       }
+
+       // bring price back up (or increase the ink) to avoid liquidation of remaining position
+       {
+        vm.store(address(pip), bytes32(uint256(1)), bytes32(uint256(1.2 * 10**18)));
+       }
+
+        // update dust
+        vm.startPrank(pauseProxy);
+
+        dss.vat.file(cfg.ilk, "dust", 11_000 * 10**45);
+
+        vm.stopPrank();
+
+        // attempt to fill the auction completely will fail now till left becomes zero
+        assert(_art(cfg.ilk,urn) == uint(10_000*10**18));
+        
+        address buyer = address(888);
+        vm.prank(pauseProxy); dss.vat.suck(address(0), buyer, 30_000 * 10**45);
+        vm.prank(buyer); dss.vat.hope(address(clip));
+        assertEq(mkr.balanceOf(buyer), 0);
+        // attempt to take the entire auction. will fail due to frob reverting
+
+        vm.prank(buyer); 
+        vm.expectRevert("Vat/dust");
+        clip.take(auctionId, 100_000 * 10**18, type(uint256).max, buyer, "");
+    }
+
+
     function _forceLiquidation(address urn) internal returns (uint256 id) {
         vm.store(address(pip), bytes32(uint256(1)), bytes32(uint256(0.05 * 10**18))); // Force liquidation
         dss.spotter.poke(ilk);

Impact

Even in an efficient liquidation market, a liquidated user's assets will be sold at a significantly lower price causing loss for the user. If there are extremely favourable conditions like control of validators for block ranges/inefficient liquidation market, then a user can self liquidate oneself to retain the collateral while evading fees/at a lowered dai price (for this the attacker will have to be the person who takes the auction once it becomes takeable). Also the setup Arbitrage Bots will loose gas fees by invoking the take function

Code Snippet

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/dba30d7a676c20dfed3bda8c52fd6702e2e85bb1/lockstake/src/LockstakeEngine.sol#L438-L454

Tool used

Manual Review

Recommendation

Use grab instead of frob to update the gem balance

WangSecurity commented 2 weeks ago

@nobodytrue please stop sending irrelevant comments, this is the discussion about escalation and this issue being valid or not. If you don't have comments for either of the sides, no need to comment.

nobodytrue commented 2 weeks ago

@WangSecurity

That statement is quite ironic, given that your judgement is predicated upon:

Circular reasoning
Invalid comments from other LSWs

While this issue is entirely valid, there seems to be a deliberate omission of the logical arguments that demonstrate why this issue is invalid. The position that you are holding can't hold up logically.

Alternatively, there may be undisclosed factors influencing this situation.

WangSecurity commented 2 weeks ago

@nobodytrue please share if you have an unanswered concern about this issue being valid and why comments from other contestants are invalid. I believe I answer each concern raised in the discussion and I will answer each new concern. I admit that some of my comments may seem unclear to other discussion participants, while they look clear to me and I’m always ready to clarify them.

So please, if you have any argument that wasn’t brought up earlier, please share it, cause all of your other comments do not add any value to the discussion and only try to manipulate the decision with false claims.

10xhash commented 2 weeks ago

@WangSecurity

No, I wouldn't say the team was aware of using the incorrect method to move funds

This is contradictory with you assuming admins are aware of the negative consequences. The negative consequences are auction execution reverting causing loss to both user and protocol. This happens solely because the incorrect method ie. frob, is used to move the funds. So an admin has to be fully aware that they are using frob to move the funds and that it can cause auction executions to revert on a dust increase and will lead to losses for both the protocol and the user

My answer is between yes and no, I wouldn't say that the team was aware it 100% will result in a loss of funds

Can you please choose the option with which you are making the decision to invalidate this issue? Because it is not possible for the team to be in a middle ground in real life. They will either know that loss of funds will happen under conditions xyz or they will be unaware that they will lose funds I think the answer to this would be Admins know that loss of funds will happen under conditions xyz because else it would be contradicting with your earlier assumption that admins are aware of the negative consequences

WangSecurity commented 2 weeks ago

The issue is not solely in using frob instead of grab, and the code would function normally using frob, unless there's increase in dust during the ongoing liquidation. Hence, I believe the root cause is not incorrectly using frob, but the admin/governance increasing dust, which makes the frob preventing the liquidation auction executed in the price range. That's why the rule applies here as it's the admin action making the code behave incorrectly, not because using one method instead of another.

The decision remains the same, accept the escalation and invalidate the issue.

10xhash commented 2 weeks ago

@WangSecurity

We have already discussed about the dust increase being a feature of the already existing makerdao system that is intended to be used. Which you have already confirmed here

I believe it's a completely viable scenario that the admin/governance would increase the dust amount during the ongoing liquidation, that's what I meant the admin mistake is no applied here. But, I believe in this case the admin should be aware of this consequence when they increase dust during liquidations

Following which you said "But, I believe in this case the admin should be aware of this consequence when they increase dust during liquidations" where the consequence is auction execution reverting causing loss of funds

So according to you admins are aware that if dust is increased during liquidations, auctions can revert and there can be loss of funds
This implies admins should know that .frob is being used and that when dust is increased .frob can revert which will cause the auction execution to revert. Because that is how the auction revert occurs

And now you say, that

No, I wouldn't say the team was aware of using the incorrect method to move funds

which is the contradiction that i am pointing out as explained above. You cannot say that the team is aware that the auctions will revert but doesn't know that it is caused by the frob method reverting

WangSecurity commented 2 weeks ago

I answered with this phrasing because you asked that specific question. You've stated several times that frob is the incorrect method to move funds and asked if the protocol is aware they use the "incorrect method", i.e. frob instead of grab. That's why I answered they're not aware, because if it's indeed the incorrect method and they were aware of it, they wouldn't use it, or it was the design decision. That's why answered "no, they're not aware".

But, it doesn't change the fact that frob becomes the incorrect method only due to admin actions during the ongoing liquidations, frob is not incorrect as a whole.

The decision remains the same, accept the escalation and invalidate the issue.

10xhash commented 2 weeks ago

@WangSecurity

Ok, I can see how the usage of incorrect word intervened there So you believe it is a conscious choice made by the admin to use the frob method to move the funds?

WangSecurity commented 2 weeks ago

Yes, I believe it was a conscious choice to use frob to move funds, since as I understand from the report and discussion, the frob works correctly unless dust is increased above the debt during the ongoing liquidation.

The decision remains, accept the escalation and invalidate the report.

10xhash commented 2 weeks ago

@WangSecurity

And why do you think that the team wants to inflict losses to the users and the protocol when increasing dust by making use of frob?

WangSecurity commented 2 weeks ago

Why in the example in the rule the protocol wants to inflict losses to the users by not allowing them to add collateral when the protocol is paused?

Excuse me, but I'm not the one to say why the team made certain decisions when designing the protocol. Since there's a check for the user's debt being higher than or equal to the dust value, the admin should be aware of possible consequences when increasing the dust.

Hence, my decision remains as previously to accept the escalation and invalidate this issue based on the admin actions rule.

10xhash commented 2 weeks ago

@WangSecurity

Why in the example in the rule the protocol wants to inflict losses to the users by not allowing them to add collateral when the protocol is paused

There would be some legit reason for that in the context as mentioned in the clarification by the previous HOJ for the two examples he demonstrated. And rational watsons or the protocol team would be able to explain the reason why, given the context

Excuse me, but I'm not the one to say why the team made certain decisions when designing the protocol

But the problem here is that this is not the design of the team. You have chosen to completely override the actual design of the protocol by making the above assumption which is never made by the team ie. team doesn't want to inflict losses to the user and the protocol by making use of frob when increasing the dust. In the actual design of the protocol, the dust increase feature has nothing to do with auctions reverting. The admins have the ability to increase/decrease the dust freely without it having any such effect on the ongoing actions. But you are completely overriding this design of the team and choosing another flawed design in which you make the dust increase feature tied with negative consequences like auctions reverting and loss of funds. This is solely your design. Hence why I was asking, why are you opting for this design?

Since there's a check for the user's debt being higher than or equal to the dust value, the admin should be aware of possible consequences when increasing the dust

Admin would indeed be aware when increasing the dust that all positions with debt below dust cannot be invoked via frob

But in a correct codebase, admin doesn't expect leftover collateral to be re-paid via the frob functionality. Which is the bug here
You are saying that because in the bugged codebase, the frob functionality is used to repay the collateral, the admin is consciously accepting the effects of the bug ie. auction reverts and loss of funds, when they are increasing the dust

I am breaking down your evidence of saying protocol was consciously accepting the consequences:

Assume the bugged codebase (ie. current one) in which invoking the dust increase can cause loss of funds, is the correct one
Now since the negative consequences of the bug will begin when the admin increases the dust, consider that the admin actually accepted these consequences when increasing the dust

AuditorPraise commented 2 weeks ago

@WangSecurity i still believe this is a low

Considering this points- https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/107#issuecomment-2293000287

protocol team's comment of this issue's likelihood of happening being extremely low with a medium impact - https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/107#issuecomment-2284535814

Panprog's escalations comment - https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/107#issuecomment-2290616548

Also accepting this issue as a medium will be unfair to other watson's who refrained from submitting admin-action issues because of the contests rules according to this comment -https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/107#issuecomment-2297359538

WangSecurity commented 2 weeks ago

But the problem here is that this is not the design of the team. You have chosen to completely override the actual design of the protocol by making the above assumption which is never made by the team ie. team doesn't want to inflict losses to the user and the protocol by making use of frob when increasing the dust. In the actual design of the protocol, the dust increase feature has nothing to do with auctions reverting. The admins have the ability to increase/decrease the dust freely without it having any such effect on the ongoing actions. But you are completely overriding this design of the team and choosing another flawed design in which you make the dust increase feature tied with negative consequences like auctions reverting and loss of funds. This is solely your design. Hence why I was asking, why are you opting for this design?

I didn't claim this to be the design of the protocol, I didn't say it's the specific design of the protocol to inflict losses on users when increasing dust. Let's look at the rule again:

An admin action can break certain assumptions about the functioning of the code. Example: Pausing collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

Here, it's the opposite, it's breaking certain assumptions about the functioning of the code, i.e. its intended design. I didn't say it's intended design to increase dust and cause losses, I said that the admin action of increasing dust can break how the code should function.

You are saying that because in the bugged codebase, the frob functionality is used to repay the collateral, the admin is consciously accepting the effects of the bug ie. auction reverts and loss of funds when they are increasing the dust

You're twisting your and my words, I didn't say the team consciously accepts the bug, I said the team was conscious of using frob, and I don't think the team was unconscious when writing the codebase.

But in a correct codebase, admin doesn't expect leftover collateral to be re-paid via the frob functionality. Which is the bug here

But, again, the frob works correctly unless the admin increases the dust to a certain level. Hence, my decision remains, that this issue arises only when the admin sets dust above the user's debt value during the ongoing liquidation, planning to accept the escalation and invalidate the issue.

nevillehuang commented 2 weeks ago

@WangSecurity

I am chiming in as a judge on sherlock. I too am confused to the rule for admin being allowed to perform ANY action causing fund loss. By sherlock rules, I agree this would automatically invalidate all issues, even if the impact is irreversible, which includes this issue.

However, I would like to point your attention to a recent finding in velocimeter where killing/pausing a gauge will cause all rewards lost.

https://github.com/sherlock-audit/2024-06-velocimeter-judging/issues/107

This finding was marked a valid high and was not escalated, presumably because it was not as high stakes as this findings (this is purely my assumption). So what is the correct decision here? Clearly this issue also share similar characteristics that hinges on an honest admin action that leads to dos of liquidation.

Imo, there are conflicting interpretation of sherlock rules that is evident in contest results, and the only beneficial change is to open a SJIP to address this rule, where an exception should be made to HONEST admin actions causing loss of funds/broken contract functionality/dos that meets sherlocks guidelines. This will clearly align and benefit security risks that needs to be fixed by protocol for such issues. This issue and the one I highlighted above will serve as good examples to this newly adjusted rule, if implemented

WangSecurity commented 2 weeks ago

It’s a very good question @nevillehuang and, to be honest, I would judge this issue from velocimeter is invalid based on this rule. But, I agree with you that this rule needs adjustments and there will be a SJIP to change it.

But, for now, my decision is as expressed before, planning to accept the escalation and invalidate the issue.

10xhash commented 2 weeks ago

@WangSecurity

I didn't say it's intended design to increase dust and cause losses, I said that the admin action of increasing dust can break how the code should function.

You also said admins were fully aware of the consequences of increasing the dust and they consciously used frob which causes the auction to revert. That is the team is aware that the codebase is written in such a way where the dust increases can result in auction reverts happening. The whole behaviour of the system including before the admin increases dust and after the admin increases the dust is what is usually called the design of the system and what I intended in the earlier message

And in the last message I pointed out that this design is solely assumed by you. In the actual design of the protocol, the dust increase feature has nothing to do with auctions reverting. The admins have the ability to increase/decrease the dust freely without it having any such effect on the ongoing actions. In this design .frob method is a bug because they don't the team doesn't accept auctions reverting when dust is increased ever. While in your design .frob is a conscious choice and increasing dust will be performed knowing auctions can revert. Hence why I was asking, why are you opting for this design?

Any rational watson auditing the codebase, will assume the correct design of the protocol ie. dust increase should not be associated with auction reverts, which is the intended design of the protocol too. If what you are assuming was indeed the design of the protocol, there are no fixes to be made. The admin knows increasing dust will cause auctions reverts which is exactly how the current bugged codebase is.

What you are assuming is incorrect and you are making your judgement based on this. If you need clarification on how the design should be, you can loop in the sponsor to ask whether they intend to use the dust increase functionality by knowing auctions can revert or whether they have to freely use the dust increase feature without having any such connection with ongoing auctions

But, again, the frob works correctly unless the admin increases the dust to a certain level

The protocol wants to have the ability to increase the dust to any level they want without it having any impact on auction executions. This is their design (And additionally, it has been made clear in past comment that increase to any value can cause this issue)

And additionally, I am attaching the lines from makerdao documentation regarding dust increase which shows they expect no such negative consequences

Vaults with a debt amount lower than the current Debt Floor will not be able to draw or payback additional DAI unless it puts their total debt over the Debt Floor, or drops it to zero. However, they will not be forcibly liquidated or suffer any immediate negative consequences

AuditorPraise commented 2 weeks ago

@WangSecurity @Evert0x i still believe this is a low

Considering this points- https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/107#issuecomment-2293000287

protocol team's comment of this issue's likelihood of happening being extremely low with a medium impact - https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/107#issuecomment-2284535814

Panprog's escalations comment - https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/107#issuecomment-2290616548

Also accepting this issue as a medium will be unfair to other watson's who refrained from submitting admin-action issues because of the contests rules according to this comment -https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/107#issuecomment-2297359538

WangSecurity commented 1 week ago

Hash, thank you again for expressing your position. But, unfortunately, my position remains.

The admin action causes auction reverts and subsequent loss of funds. This situation won't happen without admin action of increasing the dust during the ongoing liquidation.

I see the argument that "the team didn't know the bug", but this information was known only after the contest. As it always has been, the information post-contest about the bug being known/unknown is not considered. Moreover, the contest README says "Governance configurations will be set with extreme care" and the sponsor answered "No" to the Admin variables limitation question in QA (this is the information known during the contest). Hence, the following applies:

If no values are provided, the (external) admin is trusted to use values that will not cause any issues.

Hence, my decision remains, to accept the escalation and invalidate the issue.

WangSecurity commented 1 week ago

Result: Invalid Unique

sherlock-admin4 commented 1 week ago

Escalations have been resolved successfully!

Escalation status:

panprog: accepted

sherlock-audit / 2024-06-makerdao-endgame-judging

hash - Leftover dust debt can cause liquidation auction to occur at significantly lowered price #107

Leftover dust debt can cause liquidation auction to occur at significantly lowered price

Summary

Vulnerability Detail

POC

Impact

Code Snippet

Tool used

Recommendation