code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-tribe-turbo/blob/66f27fe51083f49f7935e3fe594ab2380b75dee8/src/TurboSafe.sol#L151-L155 https://github.com/Rari-Capital/solmate/blob/1205a9067ff957ef8b0b003ff9d77c20ef9f2e0b/src/mixins/ERC4626.sol#L131-L135 https://github.com/Rari-Capital/solmate/blob/1205a9067ff957ef8b0b003ff9d77c20ef9f2e0b/src/mixins/ERC4626.sol#L137-L141 https://github.com/Rari-Capital/solmate/blob/1205a9067ff957ef8b0b003ff9d77c20ef9f2e0b/src/mixins/ERC4626.sol#L143-L147 https://github.com/Rari-Capital/solmate/blob/1205a9067ff957ef8b0b003ff9d77c20ef9f2e0b/src/mixins/ERC4626.sol#L149-L153

Vulnerability details

The EIP-4626 standard says that the totalAssets() function SHOULD include any compounding that occurs from yield, which means that it's normally safe for calculations involving totalAssets() to use stale pricing data. However, the implementation of the ERC2626 contract uses totalAssets() in its calculations of other values.

Impact

The TurboSafe uses assetTurboCToken.exchangeRateStored() in its calculation of totalAssets(). The result of this calculation is used by ERC4626.previewDeposit(), ERC4626.previewMint(), ERC4626.previewWithdraw(), and ERC4626.previewRedeem(). These preview functions are called directly by the non-preview versions and therefore the TurboSafe will use a stale (i.e. an incorrect) exchange rate leading to the misspricing of assets.

Proof of Concept

Both compound-finance and Rari-Capital have the same implementations of exchangeRateCurrent() and exchangeRateStored()

    /**
     * @notice Accrue interest then return the up-to-date exchange rate
     * @return Calculated exchange rate scaled by 1e18
     */
    function exchangeRateCurrent() public nonReentrant returns (uint) {
        require(accrueInterest() == uint(Error.NO_ERROR), "accrue interest failed");
        return exchangeRateStored();
    }

    /**
     * @notice Calculates the exchange rate from the underlying to the CToken
     * @dev This function does not accrue interest before calculating the exchange rate
     * @return Calculated exchange rate scaled by 1e18
     */
    function exchangeRateStored() public view returns (uint) {
        (MathError err, uint result) = exchangeRateStoredInternal();
        require(err == MathError.NO_ERROR, "exchangeRateStored: exchangeRateStoredInternal failed");
        return result;
    }

https://github.com/compound-finance/compound-protocol/blob/ae4388e780a8d596d97619d9704a931a2752c2bc/contracts/CToken.sol#L314-L332 https://github.com/Rari-Capital/compound-protocol/blob/ae4388e780a8d596d97619d9704a931a2752c2bc/contracts/CToken.sol#L314-L332

It's used by:

    /// @notice Returns the total amount of assets held in the Safe.
    /// @return The total amount of assets held in the Safe.
    function totalAssets() public view override returns (uint256) {
        return assetTurboCToken.balanceOf(address(this)).mulWadDown(assetTurboCToken.exchangeRateStored());
    }

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

Which overrides:


    function totalAssets() public view virtual returns (uint256);

https://github.com/Rari-Capital/solmate/blob/1205a9067ff957ef8b0b003ff9d77c20ef9f2e0b/src/mixins/ERC4626.sol#L121

And is called by:

    function previewWithdraw(uint256 amount) public view virtual returns (uint256) {
        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

        return supply == 0 ? amount : amount.mulDivUp(supply, totalAssets());
    }

https://github.com/Rari-Capital/solmate/blob/1205a9067ff957ef8b0b003ff9d77c20ef9f2e0b/src/mixins/ERC4626.sol#L143-L147

previewWithdraw() is called by:

    function withdraw(
        uint256 amount,
        address to,
        address from
    ) public virtual returns (uint256 shares) {
        shares = previewWithdraw(amount); // No need to check for rounding error, previewWithdraw rounds up.

https://github.com/Rari-Capital/solmate/blob/1205a9067ff957ef8b0b003ff9d77c20ef9f2e0b/src/mixins/ERC4626.sol#L74-L79

Which is called by:

    /// @inheritdoc IERC4626RouterBase
    function withdraw(
        IERC4626 vault,
        address to,
        uint256 amount,
        uint256 minSharesOut
    ) public payable virtual override returns (uint256 sharesOut) {
        if ((sharesOut = vault.withdraw(amount, to, msg.sender)) < minSharesOut) {
            revert MinAmountError();
        }
    }

https://github.com/fei-protocol/ERC4626/blob/5b786fe0317f65f5b716f577c28092fa349c4903/src/ERC4626RouterBase.sol#L40-L50

Which is called by:

    function withdraw(IERC4626 safe, address to, uint256 amount, uint256 minSharesOut) 
        public 
        payable 
        override 
        authenticate(address(safe)) 
        returns (uint256) 
    {
        return super.withdraw(safe, to, amount, minSharesOut);
    }

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

which has direct interactions with end users.

I'm not sure whether this counts as only one instance due to the totalAssets() call, or multiple - one for each preview function exposed to end users.

Tools Used

Code inspection

Recommended Mitigation Steps

Use assetTurboCToken.exchangeRateCurrent() instead, or override the implementation of the non-preview versions of the functions to use a version of totalAssets() that uses assetTurboCToken.exchangeRateCurrent()

Joeysantoro commented 2 years ago

Turbo vaults are not yield bearing because the underlying Fuse pool collaterals do not have an interest rate model. The only way to change the exchange rate is a donation, which would be profitable for users and not exploitable.

GalloDaSballo commented 2 years ago

This is a difficult finding to judge as I believe there's no impact in this specific instance (per the sponsor reply)

However if we extend the finding to a broader scope (which I believe the sponsor made part of the contest when they asked for further scrutiny on the ERC4626 Mixin), there's definitely potential for concern.

I don't believe I have enough information to fully judge this finding so let me show @Joeysantoro a couple thoughts and see what we can agree on.

The Spec for ERC4626 says that totalAssets "should include any compounding that occurs from yield", with this statement are you trying to say that:

When calculating totalAssets, take underlying (idle assets) + invested + add pending rewards after converting to want

Or are you simply saying that totalAssets can grow over time as more rewards (not counted for totalAssets) can make the pool grow?

I believe that any attempt to calculate the economical value of some pending rewards is bound to be gamed (unless the rewards farmed are in the same asset), this can be intuitively agreed on when you compare a getAmountsOut vs actual performing a swap (which will include slippage).

As we can see in the mixin, totalAssets is being used for minting shares, so this difference in estimates, when adding the pending reward can create opportunities for arbitrage.

Because this can lead to value extraction in a fairly reliable way: e.g. totalAssets uses getAmountsOut the harvest in the strategy uses 3% slippage tolerance

This leads me to believe that there may be a medium severity finding here.

The other side of the argument is the good old "frontrunning harvest" arbitrage / exploit where any depositor could just deposit right before an harvest happens to gain access to more yield than what they contributed.

So in a way either option for totalAssets can cause issues.

Would love your thoughts here @Joeysantoro is the finding helpful in shaping the standard or the mixin? Was totalAssets comment meant to nudge developers towards including pending rewards or not?

Joeysantoro commented 2 years ago

It should depend on whether the rewards are autocompounded in the safe. The totalAssets wouldn’t include the FEI because the FEI is not automatically converted.

A great parallel is the B Protocol BAMM on liquity. It has 3 assets, LUSD, ETH, and LQTY. The ETH is sold to autocompound LUSD but the LQTY is kept so only the ETH is counted when reporting user LUSD effective balance

GalloDaSballo commented 2 years ago

Per the sponsor's reply the function totalAssets in the mixin can be based on an exchange rate and as such can be subject to value extraction.

This is in line with a medium severity finding.

Additionally, the risk is contingent on a specific implementation, off the top of my head I'd assume that a:

flat ratio (reward can be reliably be converted for X times the underlying, e.g. SUSHI -> xSUSHI)
Oracle based price (price of reward is reliably calculated by reliable oracle)

Would mitigate the risk fully or at the very risk make the discrepancy between the value of the rewards and the value of the underlying more consistent.

On the other hand an implementation that is based on a single swap would be fairly practical to code but quite manipulatable to the detriment of depositors.

Notice that Yield Farming Vaults have had the issue of having harvests front-run since their inception. This is because of the fact they typically don't account for the value of pending rewards nor they use a vesting like mechanism to make the increase in harvest be fairly distributed.

The idea of totalAssets returning the sum of the value of underlying as well as the value of rewards is in my opinion a valid attempt at trying to reduce the front-run value extraction.

However, as discussed above it can cause new types of economic exploits where based on how the value of the rewards is calculated, this value can be manipulated, tampered and ultimately exploited at the detriment of depositors.

Because this is contingent on implementation this finding cannot be of high severity.

However because economic value can be leaked, perhaps reliably, based on the specific implementation, I believe there's potential for the finding to be valid and of medium severity for the Mixin. As in: based on the implementation of the function totalAssets there can be MEV opportunities that could be exploited at the detriment of depositors.

However the warden submission is specific to the TurboSafe contract, as such I'll judge the impact on that system

GalloDaSballo commented 2 years ago

Because the warden made the case that the TurboSafe logic is flawed, I'll be judging the finding based on that scope.

While my opinion is that totalAssets can be troublesome based on implementation, I believe that in this case the impact is minimal and as the sponsor said the difference between the view function and recalculating the actual exchange rate would be meaningful exclusively if a donation where to happen.

Because of this I will downgrade the finding to Low Severity.

While in a different context there may be a risk of value extraction, I don't believe that it is the case with the Warden Provided PoC

GalloDaSballo commented 2 years ago

For the sponsor, after thinking about it, I believe that the stored version can create opportunities for Value Extraction. As the Vault is quoting that price during deposit and withdrawal.

However through various operations (perhaps an external interaction directly with the cToken) the rate can be re-synched to the "real one".

I believe this can cause leak of value very reliably and as such would not recommend using this calculation in production.

For the specific case of this contest, since there's no interest rate, there's no risk.

However, using this same implementation on a yield bearing cToken can create the above mentioned side effects, I'd recommend to instead compute the latest interest rate before issuing and redeeming new shares, to avoid opportunities for value extraction

GalloDaSballo commented 2 years ago

For the sake of judging the contest am rating this as a separate QA report because of the exceptionality of the finding and the thoroughness of the report, I believe 5/10 to be fair

code-423n4 / 2022-02-tribe-turbo-findings

QA report #21

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps