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!