search

sherlock-audit / 2023-10-mzero-judging

3 stars 2 forks source link

0xpiken - A malicious user could cause the `M` Minter to incur more penalties by calling `burnM()` #37

Closed sherlock-admin4 closed 5 months ago

sherlock-admin4 commented 5 months ago

0xpiken

medium

A malicious user could cause the M Minter to incur more penalties by calling burnM()

Summary

A malicious user could cause the Minter to incur more penalties by calling burnM()

Vulnerability Detail

Minters maintain their on-chain collateral value by calling MinterGateway#updateCollateral() periodically. They have to pay penalty if they fail to call updateCollateral() within Update Collateral Interval of the previous time they called it. The penalty will be calculated as below at the time of next updateCollateral():

π‘π‘’π‘›π‘Žπ‘™π‘‘π‘¦π‘šπ‘–π‘›π‘‘π‘’π‘Ÿ = π‘Žπ‘π‘‘π‘–π‘£π‘’π‘‚π‘€π‘’π‘‘π‘€π‘šπ‘–π‘›π‘‘π‘’π‘Ÿ π‘šπ‘–π‘ π‘ π‘’π‘‘πΌπ‘›π‘‘π‘’π‘Ÿπ‘£π‘Žπ‘™π‘ π‘π‘’π‘š π‘π‘’π‘›π‘Žπ‘™π‘‘π‘¦π‘…π‘Žπ‘‘π‘’

Any M holder can repay M owed by a Minter by calling burnM(). The penalty will be updated in burnM() if the Minter miss collateral updates within Update Collateral Interval. A malicious user could compound the penalty by periodically calling burnM():

π‘π‘’π‘›π‘Žπ‘™π‘‘π‘¦π‘šπ‘–π‘›π‘‘π‘’π‘Ÿ = π‘Žπ‘π‘‘π‘–π‘£π‘’π‘‚π‘€π‘’π‘‘π‘€π‘šπ‘–π‘›π‘‘π‘’π‘Ÿ * ((1+π‘π‘’π‘›π‘Žπ‘™π‘‘π‘¦π‘…π‘Žπ‘‘π‘’) π‘šπ‘–π‘ π‘ π‘’π‘‘πΌπ‘›π‘‘π‘’π‘Ÿπ‘£π‘Žπ‘™π‘ π‘π‘’π‘š - 1)

As we can see, a malicious user could theoretically increase the penalty of the Minter who missed π‘šπ‘–π‘ π‘ π‘’π‘‘πΌπ‘›π‘‘π‘’π‘Ÿπ‘£π‘Žπ‘™π‘ π‘π‘’π‘š of Update Collateral Interval as below: π‘Žπ‘π‘‘π‘–π‘£π‘’π‘‚π‘€π‘’π‘‘π‘€π‘šπ‘–π‘›π‘‘π‘’π‘Ÿ ((1+π‘π‘’π‘›π‘Žπ‘™π‘‘π‘¦π‘…π‘Žπ‘‘π‘’) π‘šπ‘–π‘ π‘ π‘’π‘‘πΌπ‘›π‘‘π‘’π‘Ÿπ‘£π‘Žπ‘™π‘ π‘π‘’π‘š - 1) - π‘Žπ‘π‘‘π‘–π‘£π‘’π‘‚π‘€π‘’π‘‘π‘€π‘šπ‘–π‘›π‘‘π‘’π‘Ÿ π‘šπ‘–π‘ π‘ π‘’π‘‘πΌπ‘›π‘‘π‘’π‘Ÿπ‘£π‘Žπ‘™π‘ π‘π‘’π‘š π‘π‘’π‘›π‘Žπ‘™π‘‘π‘¦π‘…π‘Žπ‘‘π‘’ = π‘Žπ‘π‘‘π‘–π‘£π‘’π‘‚π‘€π‘’π‘‘π‘€π‘šπ‘–π‘›π‘‘π‘’π‘Ÿ ((π‘šπ‘–π‘ π‘ π‘’π‘‘πΌπ‘›π‘‘π‘’π‘Ÿπ‘£π‘Žπ‘™π‘ π‘π‘’π‘š! π‘π‘’π‘›π‘Žπ‘™π‘‘π‘¦π‘…π‘Žπ‘‘π‘’2 / ((π‘šπ‘–π‘ π‘ π‘’π‘‘πΌπ‘›π‘‘π‘’π‘Ÿπ‘£π‘Žπ‘™π‘ π‘π‘’π‘š-2)! 2!) + (π‘šπ‘–π‘ π‘ π‘’π‘‘πΌπ‘›π‘‘π‘’π‘Ÿπ‘£π‘Žπ‘™π‘ π‘π‘’π‘š! π‘π‘’π‘›π‘Žπ‘™π‘‘π‘¦π‘…π‘Žπ‘‘π‘’2 / ((π‘šπ‘–π‘ π‘ π‘’π‘‘πΌπ‘›π‘‘π‘’π‘Ÿπ‘£π‘Žπ‘™π‘ π‘π‘’π‘š-3)! 3!) + ...)

Copy below codes to Integration.t.sol and run forge test --match-test test_CompoundedPenaltyByCallingBurnM

    function test_CompoundedPenaltyByCallingBurnM() external {

        _minterGateway.activateMinter(_minters[0]);

        uint256 collateral = 1_000_000e6;
        _updateCollateral(_minters[0], collateral);
        _mintM(_minters[0], 10_000e6, _alice);
        //@audit-info save the initial state
        uint snapshot = vm.snapshot();
        vm.warp(block.timestamp + 10 days);
        //@audit-info The simple-interest penalty amount for missing 10 days of collateral update.
        uint penalty = _minterGateway.getPenaltyForMissedCollateralUpdates(_minters[0]);
        //@audit-info restore the initial state
        vm.revertTo(snapshot);
        uint compoundedPenalty = 0;
        //@audit-info Calculate the compound-interest penalty amount for missing 10 days of collateral update.
        for (uint i=0; i< 10; i++) {
            vm.warp(block.timestamp + 1 days);
            //@audit-info calculate accumulative penalty
            compoundedPenalty += _minterGateway.getPenaltyForMissedCollateralUpdates(_minters[0]);
            //@audit-info Alice triggers penalty updating by repaying a tiny amount of M for _minters[0]
            vm.prank(_alice);
            _minterGateway.burnM(_minters[0], 10);
            //@audit-info the penalty will be added to activeOwedM, then reset to 0 after calling burnM() 
            assertEq(_minterGateway.getPenaltyForMissedCollateralUpdates(_minters[0]), 0);
        }
        //@audit-info The compound-interest penalty over 10 days can be around 4.5% higher than the simple-interest penalty.
        assertApproxEqAbs((compoundedPenalty - penalty) * 1e4 / penalty, 450, 5);
    }

Impact

A malicious user could cause the Minter to incur more penalties by calling burnM()

Code Snippet

https://github.com/sherlock-audit/2023-10-mzero/blob/main/protocol/src/MinterGateway.sol#L343

Tool used

Manual Review

Recommendation

When burnM() is called to repay M owed by a Minter, consider imposing a minimum threshold on the amount of M to be repaid if the Minter is subject to penalty of missing collateral update.

sherlock-admin2 commented 5 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid; medium(12)

deluca-mike commented 5 months ago

While a valid finding, this is a tradeoff between mathematical complexity (i.e. gas costs) of performing exponential penalties vs simplicity. If someone notices early that a minter missed an update, they can "poke" their account to be hit with a penalty, thus allowing for exponential penalties by doing that every update interval. If no one notices, then a linear penalty rate will apply. This actually incentives people to watch minters' accounts to ensure the max penalties are applied. After "enough" missed updates (up to governance) the minter will be removed. Therefore, the existing mechanic is favourable to the protocol to ensure people are watching and not letting a minter go derelict unnoticed.

Consider if the contract automatically computed exponential penalties, it is more likely that the minter's derelict status goes unnoticed for longer (since no one is incentivized to "poke" their account), and their actual off-chain collateral may not be enough o back their on-chain debt, and thus M.

So, long story short, it is a valid finding, but a known one that kinda works in the greater protocol's favor, that saves on gas/complexity, and won't be "fixed". I'm not sure this is an issue.

nevillehuang commented 5 months ago

@deluca-mike Was this implied as a design choice publicly during the time of the audit? I believe medium severity is appropriate otherwise.

toninorair commented 5 months ago

This is an expected by-design situation. Minter should be incentivized to update their collateral as soon as possible, especially if they missed an interval already. If there is an economic incentive for any actor in the system to catch non-compliant minters it will be done via burnM function. We put extra care to avoid penalization of minters twice for the same missed intervals to give minters time to update their collateral in the next interval after the missed one.

@nevillehuangΒ It can be considered as an issue only if minters are penalized more than once for missed collateral updates.

nevillehuang commented 5 months ago

@toninorair Did you have a chance to look at the duplicates, #5, #49 and #79? Are they duplicates? This issue wise I definitely agree with invalidating based on the following rule

  1. Design decisions are not valid issues. Even if the design is suboptimal, but doesn't imply any loss of funds, these issues are considered informational.
toninorair commented 5 months ago

@nevillehuang 5 and 49 are duplicates, 79 is not. I will double-check 79 again, but 5, 49, and 37 are the same an invalid from our perspective.