DAO Reviews

[Asking for reviews] Mushrooms Finance earning strategy on BSC to farming in Ellipsis Finance

Hey DAO reviewers!

I’m submitting this peer review request for a Smart Contract that uses BUSD to earn yield on BSC within Ellipsis Finance 3pool

Bounty
1200 $USDC OR equivalent of $MM, qualified reviewers need to submit a detail analysis with any necessary proof of findings (like test scripts with mainnet-fork) AND a publicly active twitter account of the reviewer is also required.

Code for review
StrategyEpsBUSDV1: Contract Address 0x921A43AFC2D8687765B40D2CA5D2F69F17D8AB3F | BscScan

Related components(no need to review but it helps if you are familiar with the Yearn Finance V1)
BUSD Vault:

Controller:

Goals
There is only one major goal with this review:

  1. Is the StrategyEpsBUSDV1 smart contract secure to avoid hack/funds of loss? Could attack vectors like flashloan-based or MEV-based be used to drain the assets in this strategy?

Thank you for your time!

Hey @Mushrooms!

I’ve gone through your StrategyEpsBUSDV1 Source Code

Here are my notes:

I haven’t found major exploits that would allow me to directly steal any funds.

I believe that as long as get_virtual_price from Ellipsis Finance reports back a “good” value that is not being manipulated via flashloans, this contract is secure.

I’d recommend you reach out to Ellipsis and ask them about any vectors that may change the reported value from get_virtual_price as any manipulation on it will open the strategy up for attacks.

Similar Exploits:

Vulnerabilities found:

Swap in Harvest can be frontrun

The function for swapping, called in harvest has no price checks, making it vulnerable to front-running

_swapUniswap(eps, wbnb, _eps);

Since harvest is called by onlyBenevolent, you could change harvest to receive a uint256 representing the conversion rate at the time

function harvest(uint256 conversionRate) public override onlyBenevolent {

You could then use that rate to ensure the swap is favourable.

I’ve seen similar farms “not care” about this, by swapping very often with very small quantities, this may be good enough based on gas costs

Slippage check in deposit always returns true, deposit can be frontrun

The function for deposits

    function deposit() public override {
        uint256 _wantAmt = IERC20(want).balanceOf(address(this));
        if (_wantAmt > 0 && checkSlip(_wantAmt)) {
            uint256[3] memory amounts = [_wantAmt, 0, 0]; 
            ICurveFi_3(pool3EPS).add_liquidity(amounts, 0);
        }
		
        _depositLP();
    }

calls checkSlip to prevent getting an unfavourable deposit rate

However checkSlip is simply calling return ICurveFi_3(pool3EPS).calc_token_amount(amounts, true) >= maxSlip;

calc_token_amount is an external function used to “preview” your deposit, and If you calculate slippage inside the tx block, due to the atomic nature of transactions, the slippage check will always return true even if you are being sandwiched

Solution: have deposit receive the result from calc_token_amount, or have deposit just deposit in the vault and then have a trusted entity add_liquidity with externally calculated slippage protection.

Basic Improvement:

Set want to immutable

I’d recommend changing
address public want;
to immutable since it will never change
https://docs.soliditylang.org/en/v0.6.12/contracts.html#constant-and-immutable-state-variables

Double Slippage may not be necessary

The function function _withdrawSome(uint256 _amount) internal override returns (uint256) {
Checks for slippage twice

_required3EPS = _required3EPS.mul(DENOMINATOR.add(slippageProtectionOut)).div(DENOMINATOR);// try to remove bit more

Then it calculates the min_out like so:
uint256 maxSlippage = _required3EPS.mul(DENOMINATOR.sub(slippageProtectionOut)).div(DENOMINATOR);

I’m thinking you don’t need the initial slippage added to _required3EPS as the “real” slippage calculation is done when you calculate uint256 maxSlippage

Note that also these calculations are vulnerable to frontrunning, however, as long as get_virtual_price is benevolent, you should be fine

Hope this helps!

@AlexTheEntreprenerd Thank you for this detail analysis.

Yes, our BSC strategy does the harvest() every a few hours to make the profit small and frequently claimed to avoid large volume swapping and the earn() is restricted to onlyKeeper() in BUSD vault (line 1319 in the vault code on bscscan) thus the deposit() in this strategy actually is protected within limited callers.

And we’ve confirmed with Ellipsis team that its get_virtual_price() should be safe to use which maintains same semantic as in Curve to protect against manipulation. Telegram: Contact @ellipsisfinance

Thank you again for the review and please let me know preferrable payment recipient address via twitter message.

Awesome! Sent you Address on Twitter!

1 Like

Payment tx hash sent via twitter message. Thank you ser again for this analysis

I confirm receiving the bounty, thank you!

1 Like