araj - `ApprovedEarner` can still earn `EarnerRate` even after removal from `EARNERS_LIST`

[sherlock-admin2]

araj

high

ApprovedEarner can still earn EarnerRate even after removal from EARNERS_LIST

Summary

ApprovedEarner can still earn EarnerRate even after removal from EARNERS_LIST as there is no way to stop a RemovedEarner from earning.

Vulnerability Detail

Only approvedEarners are can earn the earnerRate by calling MToken::startEarning() which checks an Earner is approved(ie in EARNERS_LIST) or not & mark isEarning = true

mBalance_.isEarning = true;

To stop earning EarnerRate, Earner needs to call MToken::stopEarning() which sets isEarning = false

mBalance_.isEarning = false;

But the problem is, when an Earner is removed from EARNERS_LIST, it's on Earner Goodwill to call MToken::stopEarning() as this function stops only caller of this functions ie msg.sender from earning EarnerRate(ie isEarning = false).

   function stopEarning() external {
    @>    _stopEarning(msg.sender);
    }

There is no other way to stop an RemovedEarner from earning EarnerRate.

//Here is the POC

function test_stillEarningAfterRemoval() external {
        //Setting initial parameters
        _mToken.setLatestRate(_earnerRate);
        _mToken.setTotalNonEarningSupply(1_000);
        _mToken.setInternalBalanceOf(_alice, 1_000);

        //Adding _alice to EARNERS_LIST
        _registrar.addToList(TTGRegistrarReader.EARNERS_LIST, _alice);

        //_alice started earning EarnerRate
        vm.prank(_alice);
        _mToken.startEarning();

        //checking variables
        assertEq(_mToken.balanceOf(_alice), 999);
        assertEq(_mToken.isEarning(_alice), true);
        assertEq(_mToken.totalNonEarningSupply(), 0);
        assertEq(_mToken.principalOfTotalEarningSupply(), 909);

        //Removing _alice from EARNERS_LIST
        _registrar.removeFromList(TTGRegistrarReader.EARNERS_LIST, _alice);

        //Skipping 1 year for 10% APY(for testing)
        skip(365 days);

        //Here we can see balanceOf(_alice) is Greater than 999 ie initial balance
        //(will be 1105 -> alice balance after 1 year)
        assertGt(_mToken.balanceOf(_alice), 999);
        assertEq(_mToken.isEarning(_alice), true);
        assertEq(_mToken.totalNonEarningSupply(), 0);
        assertEq(_mToken.principalOfTotalEarningSupply(), 909);
    }

Impact

RemovedEarner will keep earning EarnerRate forever

Code Snippet

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

https://github.com/sherlock-audit/2023-10-mzero/blob/main/protocol/src/MToken.sol#L106C4-L108C6

https://github.com/sherlock-audit/2023-10-mzero/blob/main/protocol/src/MToken.sol#L262C2-L288C6

https://github.com/sherlock-audit/2023-10-mzero/blob/main/protocol/src/MToken.sol#L294C2-L318C6

Tool used

Manual Review

Recommendation

Take address as parameter in stopEarning(), check if address is removed from EARNERS_LIST & also check that address is earning

Duplicate of #33

[deluca-mike]

This is actually a valid issue. I introduced a PR that removed unnecessary functionality but that additionally erroneously introduced this bug that only account holders can stop earning on their own account. Originally it was supposed to be that anybody can stop earning on an account that is no longer in the earners list.

[PierrickGT]

It is actually a valid issue since as the watson mentioned, stopEarning() is only callable by the earner itself and if he is removed from the TTG Registrar earner list, he has no incentive to call this function.

The issue has been fixed in this PR by adding stopEarning(address account) that is callable by anyone: https://github.com/MZero-Labs/protocol/pull/162

[sherlock-admin2]

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

takarez commented:

valid; medium(2)