`totalFeiBoosted` vars can desync in `TurboSafe` and `TurboMaster`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-tribe-turbo/blob/66f27fe51083f49f7935e3fe594ab2380b75dee8/src/TurboSafe.sol#L230

Vulnerability details

Impact

The TurboSafe and TurboMaster contracts each keep track of totalFeiBoosted, getTotalFeiBoostedForVault, and Master keeps track of getTotalBoostedAgainstCollateral in addition. For the TurboMaster contract, these values should be the sum of the corresponding values across all vaults.

However, it's possible to desync this when doing TurboSafe.less:

function less(ERC4626 vault, uint256 feiAmount) external nonReentrant requiresLocalOrMasterAuth {
    // @audit decreases by feiAmount
    getTotalFeiBoostedForVault[vault] -= feiAmount;
    totalFeiBoosted -= feiAmount;

    // @audit can be set to a different, smaller value
    if (feiAmount > feiDebt) feiAmount = feiDebt;

    // @audit can be called with the smaller feiDebt value now => desync
    master.onSafeLess(asset, vault, feiAmount);
}

Assume fei yield is reinvested in a safe and, because of zero-interest-rate borrowing, the borrowed + slurped fei is greater than the debt. Then lessing the entire getTotalFeiBoostedForVault[vault] amount will trigger the feiAmount > feiDebt case. The Master still acts like the vault is boosted when it's not.

The values in TurboMaster will be higher than the values for the vault. When trying to boost again, the cap can be reached in TurboMaster.onSafeBoost's booster.canSafeBoostVault call and the function reverts because of this wrong number.

Note that the TurboMaster.getTotalBoostedAgainstCollateral value will also be inflated and this value is vault-independent. At some point, entire collateral asset classes cannot be used to boost anymore as they wrongly reach the cap because of this bug. The borrow cap would need to be increased every time and it does not reflect the actual boosted fei anymore.

Recommended Mitigation Steps

The TurboSafe.less should call master.onSafeLess(asset, vault, feiAmount); with the original feiAmount, not the reduced feiDebt.

[Joeysantoro]

This is definitely an issue, disputing the severity and suggesting it be medium due to the fact that it can be easily mitigated by adjusting caps and is costly to repeatedly execute

[Joeysantoro]

https://github.com/fei-protocol/tribe-turbo/pull/65

[GalloDaSballo]

Duplicate of #55

[GalloDaSballo]

I had to think about this finding for quite some time, while this reasoning won't make it to the report I believe it's worth sharing why I ultimately agreed with Medium Severity over High.

The biggest argument for High Severity is that the warden was able to break protocol invariants. Ultimately the system has been tricked into a state that is not consistent with reality. This can make the finding be of high severity.

In this specific case, the impact is a minor denial of service that applies exclusively to borrowing new collateral. Existing deposits are not at risk. Liquidations are not blocked. Collateral can still flow out of the system properly.

The one thing that's being denied through this findings are new borrows.

The mitigation on the sponsor side is to simply increase caps, to allow further borrowing.

In conclusion, the finding was very well researched, the protocol invariants have been broken, however in doing so no funds nor value has been leaked. So I believe Medium Severity to be more appropriate.