extra safety in distributeYieldForTreasuryAndReturnMarketAllocation

[code423n4]

Handle

gpersoon

Vulnerability details

Impact

The function distributeYieldForTreasuryAndReturnMarketAllocation can revert if totalRealized > totalHeld (as noted in the comments)

Although this very unlikely, the consequence will be large because distributeYieldForTreasuryAndReturnMarketAllocation is called by
_claimAndDistributeYieldThenRebalanceMarket , which is call by_updateSystemStateInternal.
So the entire market would no longer function.

As this edge case is easy to solve it seem like a now brainer to do.

Proof of Concept

//https://github.com/code-423n4/2021-08-floatcapital/blob/main/contracts/contracts/YieldManagerAave.sol#L179
function distributeYieldForTreasuryAndReturnMarketAllocation(
..
uint256 totalHeld = aToken.balanceOf(address(this));
uint256 totalRealized = totalValueRealizedForMarket + totalReservedForTreasury + amountReservedInCaseOfInsufficientAaveLiquidity;
if (totalRealized == totalHeld) {
return 0;
}
// will revert in case totalRealized > totalHeld which should be never.
uint256 unrealizedYield = totalHeld - totalRealized;

Tools Used

Recommended Mitigation Steps

replace
if (totalRealized == totalHeld) { return 0; }
with
if (totalRealized >= totalHeld) { return 0; }

[JasoonS]

Although this very unlikely

It isn't unlikely, it is impossible. Using totalRealized >= totalHeld would be a fix, but really we should have.

if (totalRealized == totalHeld) {
return 0;
}
assert(totalRealized < totalHeld);

totalRealized < totalHeld is an invariant in our code (assuming that aave doesn't give negative interest rates in the future...)

[moose-code]

Correct. I was actually responsible for explicitly changed the line

if (totalRealized >= totalHeld) { return 0; }

to:
if (totalRealized == totalHeld) {

It would be terrible IMO if our code simply ignored this invariant and returned zero.

The reason I didn't add this assert that @JasoonS mentioned
assert(totalRealized < totalHeld);

Was since in built safemath should revert in the same way an assert would. An assert would be more explicit.

[JasoonS]

Safe math to the rescue once again 🤗

[0xean]

downgrading to 0.