Treasury cannot claim COMP tokens & COMP tokens are stuck

[code423n4]

Handle

cmichel

Vulnerability details

The TreasuryAction.claimCOMPAndTransfer function uses pre- and post-balances of the COMP token to check which ones to transfer:

function claimCOMPAndTransfer(address[] calldata cTokens)
    external
    override
    onlyManagerContract
    nonReentrant
    returns (uint256)
{
    // Take a snasphot of the COMP balance before we claim COMP so that we don't inadvertently transfer
    // something we shouldn't.
    uint256 balanceBefore = COMP.balanceOf(address(this));
    // @audit anyone can claim COMP on behalf of this contract and then it's stuck. https://github.com/compound-finance/compound-protocol/blob/master/contracts/Comptroller.sol#L1328
    COMPTROLLER.claimComp(address(this), cTokens);
    // NOTE: If Notional ever lists COMP as a collateral asset it will be cCOMP instead and it
    // will never hold COMP balances directly. In this case we can always transfer all the COMP
    // off of the contract.
    uint256 balanceAfter = COMP.balanceOf(address(this));
    uint256 amountClaimed = balanceAfter.sub(balanceBefore);
    // NOTE: the onlyManagerContract modifier prevents a transfer to address(0) here
    COMP.safeTransfer(treasuryManagerContract, amountClaimed);
    // NOTE: TreasuryManager contract will emit a COMPHarvested event
    return amountClaimed;
}

Note that anyone can claim COMP tokens on behalf of any address (see Comptroller.claimComp).
An attacker can claim COMP tokens on behalf of the contract and it'll never be able to claim any compound itself.
The COMP claimed by the attacker are stuck in the contract and cannot be retrieved.
(One can eventually get back the stuck COMP by creating a cCOMP market and then transferring it through transferReserveToTreasury.)

Recommended Mitigation Steps

Don't use pre-and post-balances, can you use the entire balance?

[jeffywu]

Duplicate of #192, dispute as a high risk bug. Would categorize this as medium risk.

There is no profit to be gained by doing this from the attacker besides denial of service. The protocol could simply upgrade to regain access to the tokens. We will fix this regardless.

[pauliax]

Very good find.

It is a tough decision if this should be classified as High or Medium severity. An exploiter cannot acquire those assets, and the contracts are upgradeable if necessary, however, I think this time I will leave it in favor of wardens who both are experienced enough and submitted this as of high severity:
3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).