code-423n4 / 2021-04-basedloans-findings

0 stars 1 forks source link

`refreshCompSpeedsInternal` should only update `isComped` #34

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

@cmichelio

Vulnerability details

Vulnerability Details

The last for loop iterates over all markets and sends a CompSpeedUpdated event for all of them, even though some of these markets don't even have rewards (!isComped).

for (uint i = 0; i < allMarkets_.length; i++) {
    CToken cToken = allMarkets[i];
    uint newSpeed = totalUtility.mantissa > 0 ? mul_(compRate, div_(utilities[i], totalUtility)) : 0;
    compSpeeds[address(cToken)] = newSpeed;
    emit CompSpeedUpdated(cToken, newSpeed);
}

Impact

Performing the computation and sending events for all of these markets costs more gas and sending a CompSpeedUpdated event for markets that don't even have rewards feels wrong and could cause issues in off-chain scripts that don't expect it.

Recommended Mitigation Steps

Perform an if (markets[address(cToken)].isComped) check like in the second for loop and only update these markets.

ghoul-sol commented 3 years ago

Last loop iterates over all markets but not comped markets will have speed set to 0. Emitted events might be useful because they indicate that compSpeed was updated and for no comped market is 0. It's a confirmation that data is fresh and config is correct.

cemozerr commented 3 years ago

I agree here with the sponsor's view that CompSpeedUpdated event with speed set to 0 is a good indicator that data is fresh and config is correct. Thus, I'm marking it as invalid.