nirohgo - LockStakeEngine::OnRemove wrong fee amount calculation

[sherlock-admin2]

nirohgo

Medium

LockStakeEngine::OnRemove wrong fee amount calculation

Summary

The LockStakeEngine::onRemove function (called when an auction ends) collects the wrong amount of exit fee from the left collateral

Vulnerability Detail

The LockStakeEngine::onRemove function is called by the LockstakeClipper contract when the auction ends. It passes the amount of collateral sold and the amount left, and the engine handles the leftovers in the following way: if any collateral is left after the auction, the engine collects the exit fee due for the sold collateral from the leftover-collateral. Whatever is left after that, is put back into the URN:

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);
    }

Note that the calculation used to determine the fee is sold * fee / (WAD - fee) (the burned amount is the minimum between the result and the leftover collateral). However, this calculation is wrong. The fee should be calculated using this formula: uint256 burn = wad * fee_ / WAD;, in the same way it is done in the free function here:https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/dba30d7a676c20dfed3bda8c52fd6702e2e85bb1/lockstake/src/LockstakeEngine.sol#L373 because `fee` is given as a ratio where WAD represents 1 as can be seen here.

Numeric Example

1. A URN with total collateral (Mkr) worth of $10500 is liquidated
2. At the end of the auction $10000 worth of collateral is sold and $500 worth is left
3. Assume the fee is 15% (quoting from the lockstake folder readme: "(where fee will typically be 15%)" )
4. The due fee is $10000*0.15 = $1500.
5. The fee calculated is $10000*0.15/(1-0.15) = $1764
6. This means that an amount of Mkr worth $1764-$1500 = $264 is burned from the user's leftover Mkr unnecessarily causing them a loss.
7. In percentage terms: the user loses 264/10500 = 2.52% of the original urn collateral
8. If the exit fee increases above 20.7% , the user loses more than %5 of the collateral value

Root cause

Wrong calculation of fee in LockStakeEngine::OnRemove: sold fee / (WAD - fee) instead of sold fee / WAD

Impact

As can be seen in the numeric example above, this error causes any liquidated user with enough leftover a loss of 2.5% typically and possibly above 5% depending of the value of fee.

Code Snippet

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

Tool used

Manual Review

Recommendation

Fix the formula in the code above to the correct form: burn = _min(sold * fee / WAD , left);

Duplicate of #20

[sunbreak1211]

No, it is wrong, you shouldn't be calculating the fee as it is in free.

When you free an amount wad, the burn value is defined as wad * fee / WAD. Then the user receives wad - burn (freed). In the case of finishing an auction, sold is equivalent to freed. So if you use the classic operation, you wouldn't be properly calculating the fee value. basically burn = (sold + burn) * fee / WAD

[nirohgo]

Escalate

In the case of finishing an auction, sold is equivalent to freed. So if you use the classic operation, you wouldn't be properly calculating the fee value. basically burn = (sold + burn) * fee / WAD

This comment implies that a liquidated user should be charged an exit fee based on a calculation that considers the Mkr amount sold in the liquidation as the post-fee amount of exited Mkr for which a fee should be paid. This is not only incorrect (the actual total amount that exited the LSE as a result of the liquidation is 'sold') but imposes a de-facto exit fee rate (on the amount that actually exits the LSE) that is arbitrarily larger than the actual fee rate.

The ratio between the exit fee applied in onRemove and the defined fee is 1/(1-fee), which grows to infinity as fee approaches 1, making it an undocumented, hidden and arbitrary ""extra liquidation penalty"" and therefore should be treated as a real loss of funds for the user that can easily exceed 5% of the urn Mkr value.

For example: If the exit fee was 0.91 (91%), the exit fee rate the user would be paying for the amount of Mkr sold in the liquidation will be 0.91/(1-0.91) = 10.111 or 1011% of the amount that actually left the LSE. This clearly exceeds any reasonable interpretation of an exit fee defined as a fixed rate out of the Mkr amount a user exits from the LSE.

The fact that the exit fee is applied this way when a user is being liquidated is not a valid argument to justify its application in a way that wrongs the user. The protocol has a clearly stated liquidation penalty (chop) charged at the start of the auction in dog.bark. Anything charged beyond that is an undocumented hidden extra penalty and therefore an unwarranted loss for the user, making this finding valid.

Finally, This finding was marked a dup of #20 (together with #98), however #20 has a wrong root cause (It states that the burned amount should be sold * (WAD - fee) / (WAD) where in fact it should be sold * fee / WAD, which is why I'm escalating this finding and not #20.

[sherlock-admin3]

Escalate

In the case of finishing an auction, sold is equivalent to freed. So if you use the classic operation, you wouldn't be properly calculating the fee value. basically burn = (sold + burn) * fee / WAD

This comment implies that a liquidated user should be charged an exit fee based on a calculation that considers the Mkr amount sold in the liquidation as the post-fee amount of exited Mkr for which a fee should be paid. This is not only incorrect (the actual total amount that exited the LSE as a result of the liquidation is 'sold') but imposes a de-facto exit fee rate (on the amount that actually exits the LSE) that is arbitrarily larger than the actual fee rate.

The ratio between the exit fee applied in onRemove and the defined fee is 1/(1-fee), which grows to infinity as fee approaches 1, making it an undocumented, hidden and arbitrary ""extra liquidation penalty"" and therefore should be treated as a real loss of funds for the user that can easily exceed 5% of the urn Mkr value.

For example: If the exit fee was 0.91 (91%), the exit fee rate the user would be paying for the amount of Mkr sold in the liquidation will be 0.91/(1-0.91) = 10.111 or 1011% of the amount that actually left the LSE. This clearly exceeds any reasonable interpretation of an exit fee defined as a fixed rate out of the Mkr amount a user exits from the LSE.

The fact that the exit fee is applied this way when a user is being liquidated is not a valid argument to justify its application in a way that wrongs the user. The protocol has a clearly stated liquidation penalty (chop) charged at the start of the auction in dog.bark. Anything charged beyond that is an undocumented hidden extra penalty and therefore an unwarranted loss for the user, making this finding valid.

Finally, This finding was marked a dup of #20 (together with #98), however #20 has a wrong root cause (It states that the burned amount should be sold * (WAD - fee) / (WAD) where in fact it should be sold * fee / WAD, which is why I'm escalating this finding and not #20.

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.

[sunbreak1211]

The exit fee applies for any amount of MKR that leaves the system. What it is sold in an auction should follow the same conditions. The way to make it equivalent, is treating the sold amount in the same way than what a user would receive in a free action (freed). Therefore the correct math is the applied one and not what you are suggesting. This was even purposely changed after discovered during internal reviews: https://github.com/makerdao/lockstake/pull/1#discussion_r1486106625

This is not an issue. We know that this one has actually confused several people, including auditors and some other Sherlock participants, but after making this same explanation it has always been acknowledged as understood.

[nirohgo]

Hi @sunbreak1211 , Thanks but you've basically repeated the claim that 'sold' should be equated to 'freed' (post fee) without justifying it or addressing the reasoning above (and in #98) of why it shouldn't be (and that doing so would cause unwarranted losses to users). That this came as a result of an internal review doesn't matter, an internal review might be a source of an error as well. Ofcourse, as the sponsor you're free to keep whichever implementation you choose, but for the purpose of this contest what matters is weather or not this implementation is inline with the way the exit fee is defined (or a reasonable user's understanding of it).

[sunbreak1211]

There is nothing to justify, this is how it works and how you make both actions even. Let's say you deposit 100 MKR having an exit fee of 91% (which btw ofc it isn't in the order of magnitude of what will be). If you are freeing all, you will get 9 MKR and 91 will be burnt (then it is still burning 10.1x what you are receiving). We are finishing our arguments here as there is clear evidence that this was the intended functionality and honestly we can't keep using our time to argument that.

[WangSecurity]

I agree with the sponsor. The protocol works exactly as it's expected and intended. Hence, I don't see it as a loss of funds because the fee should be calculated in this way and there's no mistake. Thus, this should remain as it is, planning to reject the escalation and leave the issue as it is.

[nirohgo]

Let's say you deposit 100 MKR having an exit fee of 91% (which btw ofc it isn't in the order of magnitude of what will be). If you are freeing all, you will get 9 MKR and 91 will be burnt (then it is still burning 10.1x what you are receiving).

If the same user's 100Mkr were sold in a liquidation acution they would be charged a fee of 1011MKR instead of 91, that's not making these actions even.

[WangSecurity]

If you still believe it's correct, then create a coded POC, but based on this comment I believe your understanding is incorrect and the protocol works as intended.

The decision remains as is, reject the escalation and leave the issue.

[nirohgo]

If you still believe it's correct, then create a coded POC, but based on https://github.com/sherlock-audit/2024-06-makerdao-endgame-judging/issues/106#issuecomment-2297216423 U believe your understanding is incorrect and the protocol works as intended.

The decision remains as is, reject the escalation and leave the issue.

@WangSecurity apparently you didn't understand either side is this escalation. Even the sponsor doesn't dispute my statement is correct, they're just claiming this is how it should be. You don't need a POC of that, just elementary school level math.

[WangSecurity]

Firstly, I'm asking you to be more respectful.

Secondly, when I said your understanding was incorrect I meant you didn't understand the protocol works as intended and after the sponsor's reply, you said that the fee charged is 1011 MKR instead of 91. As far as I understand, in the example of 100 MRK with 91% exit fee, the charged fee is indeed 91 MKR, not 1011 MKR. The user gets 9 MKR, which makes the burnt amount 10.1X, i.e. 91 MKR, not 1011.

Thirdly, I asked you to provide a POC because the sponsor said the fee would be correctly calculated as it should be, while you continued to claim it would be 1011 instead of the expected 91. That's why I asked to prove your claims with the POC.

The decision remains, reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Invalid Duplicate of #20

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• nirohgo: rejected