bin2chen - claimCOMPAndTransfer() COMP may be locked into the contract

[sherlock-admin]

bin2chen

high

claimCOMPAndTransfer() COMP may be locked into the contract

Summary

Malicious users can keep front-run claimCOMPAndTransfer() to trigger COMPTROLLER.claimComp() first, causing netBalance in claimCOMPAndTransfer() to be 0 all the time, resulting in COMP not being transferred out and locked in the contract

Vulnerability Detail

claimCOMPAndTransfer() use for "Claims COMP incentives earned and transfers to the treasury manager contract" The code is as follows:

    function claimCOMPAndTransfer(address[] calldata cTokens)
        external
        override
        onlyManagerContract
        nonReentrant
        returns (uint256)
    {
        uint256 balanceBefore = COMP.balanceOf(address(this));
        COMPTROLLER.claimComp(address(this), cTokens);
        uint256 balanceAfter = COMP.balanceOf(address(this));

        // NOTE: the onlyManagerContract modifier prevents a transfer to address(0) here
        uint256 netBalance = balanceAfter.sub(balanceBefore);   //<-------@only transfer out `netBalance`
        if (netBalance > 0) {
            COMP.safeTransfer(msg.sender, netBalance);
        }

        // NOTE: TreasuryManager contract will emit a COMPHarvested event
        return netBalance;

From the above code, we can see that this method only turns out the difference value netBalance But COMPTROLLER.claimComp() can be called by anyone, if there is a malicious user front-run this transcation to triggers COMPTROLLER.claimComp() first This will cause thenetBalance to be 0 all the time, resulting in COMP not being transferred out and being locked in the contract.

The following code is from Comptroller.sol

https://github.com/compound-finance/compound-protocol/blob/master/contracts/Comptroller.sol

    function claimComp(address holder, CToken[] memory cTokens) public { //<----------anyone can call it
        address[] memory holders = new address[](1);
        holders[0] = holder;
        claimComp(holders, cTokens, true, true);
    }

Impact

COMP may be locked into the contract

Code Snippet

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/external/actions/TreasuryAction.sol#L118C4-L123

Tool used

Manual Review

Recommendation

Transfer all balances, not using netBalance

[Jiaren-tang]

.

[Jiaren-tang]

Escalate for 10 USDC. do not think this can be a high vulnerability because of the following reason

this is a medium because first, the protocol can use flashbot to avoid frontrunning

this is loss of reward not lose of user fund

this griefing attack has no gain from attacker at all

https://docs.sherlock.xyz/audits/judging/judging

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

we consider the attack is cost high becaues the attacker needs to monitor the pending claim transaction from notional side while has no economic gain

[sherlock-admin]

Escalate for 10 USDC. do not think this can be a high vulnerability because of the following reason

this is a medium because first, the protocol can use flashbot to avoid frontrunning

this is loss of reward not lose of user fund

this griefing attack has no gain from attacker at all

https://docs.sherlock.xyz/audits/judging/judging

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

we consider the attack is cost high becaues the attacker needs to monitor the pending claim transaction from notional side while has no economic gain

You've created a valid escalation for 10 USDC!

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.

[0xleastwood]

Escalate for 10 USDC. do not think this can be a high vulnerability because of the following reason

this is a medium because first, the protocol can use flashbot to avoid frontrunning

this is loss of reward not lose of user fund

this griefing attack has no gain from attacker at all

https://docs.sherlock.xyz/audits/judging/judging

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

we consider the attack is cost high becaues the attacker needs to monitor the pending claim transaction from notional side while has no economic gain

I disagree, COMP rewards continuously accrue over time and hence this function can be called at any time to lose rewards. This attack is not expensive at all to perform and can be profitable in other ways that aren't immediately apparent. The difference between high and medium risk within the context of Sherlock's judging is There is a viable scenario (even if unlikely) vs This vulnerability would result in a material loss of funds, and the cost of the attack is low (relative to the amount of funds lost). It is clear that this fits the criteria of the latter.

[Trumpero]

Agree with the comment by Leastwood, this issue should be high

[hrishibhat]

Result: High Has duplicates Given that the rewards are an integral part of protocol these would be stuck in the contract due to the exploit considering this as a valid high

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• ShadowForce: rejected