beefyfinance/beefy-contracts

Public repo for the community devs to advance the Beefy protocol.

beefy-contracts's Issues

Propose valid candidate for Cake community pools

Last week I proposed a candidate upgrade for the STAX-CAKE/NAR-CAKE/NYA-CAKE vaults to decrease their slippage. For some reason the upgrade didn't work and with the app + api issues that followed, I wasn't able to focus on them.

Even though they're pretty small vaults, it would be good to make sure we deliver the APY that we display. Debugging what went wrong and/or proposing a candidate with the correct custom routes would make this happen.

Create automated test to verify vault upgrades ($1000)

Context

We have close to 150 vaults and growing. There are times when we have to upgrade multiple vaults at a time, and there might come a time when we have to upgrade +50 in a short period of time. An example would be if we have to change the BIFI rewards pool address, or the treasury address, or add an improvement to all strats.

Right now the process to upgrade a strategy is as follows: When the approval time has passed and the candidate is ready
to replace the old strat, we first do a dry run of the upgrade using a ganache localnet.

We check that:
1- Balances are transfered correctly.
2- We can deposit/withdraw.
3- Harvests can happen and generate earnings for stakers.

If everything looks good, we run the upgrade on mainnet.

Problems

This is quite slow to do. And if we have to do it for several vaults in a row, we could miss a detail somewhere.

Solution

Having an automated test that does this checks would increase our comfort when upgrading vaults. Some of these vaults have $10M-$20M and those numbers are only going to get larger.
It would still be a good idea to run the manual runs at first, but the automated tests would still be very beneficial. The good thing is that these are integration tests which could be re-used for 100% of our vaults.

Specs

The test at VaultUpgrade.test.js would have to receive a vault address and:

1. Simulate an upgrade using our ganache fork yarn net.
2. Check that balances transfered correctly.
3. Check that it's possible to deposit.
4. Check that a harvest gives funds to the correct parties.
5. Check that a withdraw gets back the expected funds.

Stop accepting 100% slippage on strategy swaps

Context

Our strategies always send a 0 as amountOutMin when swapping. This means that we're accepting any slippage that the exchange offers.

Problems

This means that our vaults are:

1. Easier to be exploited by frontrunning bots.
2. Can burn out a ton of rewards depending on the route.

Solution

1. Having a healthy default cap on accepted slippage.
2. Being able to move that cap within a certain range.

Update link in readme

the link for vault in 5. Update the app is pointing to the archived repo

Upgrade BeefyVaults to allow EIP-2612 signed approvals

I noticed a few DApps now make use of a new spending permission system outlined in EIP-2612, which involves signing a message in your wallet instead of a separate approval transaction (more details here: https://soliditydeveloper.com/erc20-permit).

Obviously, the token being spent must support this approval style, namely, it must implement the following three methods:

function permit(address owner, address spender, uint value, uint deadline, uint8 v, bytes32 r, bytes32 s) external
function nonces(address owner) external view returns (uint)
function DOMAIN_SEPARATOR() external view returns (bytes32)

It does appear that at least SpiritSwap LP tokens on Fantom DO implement this approval style (for example, the USDC-fUSDT pair. Not sure if this only goes for newer pairs and what other exchanges have implemented this, but I will do some research and add a list here.

Of course, the app would need some updates as well in order to support this approval style (and autodetect when it is available), but more importantly, the vaults would need to support this first. I found a possible implementation on a YieldYak vault (license is MIT):

 /**
     * @notice Deposit using Permit
     * @param amount Amount of tokens to deposit
     * @param deadline The time at which to expire the signature
     * @param v The recovery byte of the signature
     * @param r Half of the ECDSA signature pair
     * @param s Half of the ECDSA signature pair
     */
    function depositWithPermit(
        uint256 amount,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external override {
        depositToken.permit(msg.sender, address(this), amount, deadline, v, r, s);
        _deposit(msg.sender, amount);
    }

A potential depositAllWithPermit() function would look similar.

In the app, the following steps would be required:

1. Detect if deposit token (i.e. want) supports EIP-2612 approvals.
2. Produce permission message and ask user to sign it
3. Instead of calling deposit(amount) or depositAll(), call the _WithPermit version, using the appropriate parameters.

While I don't think this saves any gas or transaction fees (after all, the permit() function still calls approve(), and must additionally decode the signed message), it is a nice quality-of-life improvement and does improve security, since no permanent approvals are required.

Would it be possible to include this in the next iteration of the vault contract?

Stop charging fees when a strat has been paused.

What do we think about adding a check to see if a strat is paused and from that state, don't charge a withdrawal fee.

There are three times where the strats are paused and I think this handles all of them:

1. Vault reached EOL for some reason. We don't want to punish users for withdrawing.
2. Vault is in a temporary panic() state without working. Users should be able to move to a different vault freely.
3. Vault is paused() but funds are still deposited at the farm. There's no risk of arbitrage here as noone can deposit from a paused state. So there's no harm in 0% withdrawal fees.

    function withdraw(uint256 _amount) external {
       ...

        if (tx.origin == owner() || paused()) {
            IERC20(lpPair).safeTransfer(vault, pairBal);
        } else {
            uint256 withdrawalFee = pairBal.mul(WITHDRAWAL_FEE).div(WITHDRAWAL_MAX);
            IERC20(lpPair).safeTransfer(vault, pairBal.sub(withdrawalFee));
        }
    }

If we like it, I can add it to all strats.

beets rounding error causes vault to throw at >0 if-statement

encountered an issue pertaining to this line with the current beets-cre8r vault:

beefy-contracts/contracts/BIFI/strategies/Beethovenx/StrategyBeethovenxDual.sol

Line 135 in 481fd6e

if (outputBal > 0 || rewardBal > 0) {

I setup the vault to accept beets outputs in case they were eventually bribed for by cre8r. however, while current beets emissions are nominally zero, because of "rounding error" (according to beets admin) 1gwei beets is emitted every day or so. because the strat contract tries to swap beets whenever beets > 0, when it detects 1gwei beets it tries to trade, and as a consequence throws.

its unclear what a solution might be. any lower bound on trade would be arbitrary since its difficult to predict the price range at which the want token will swap against beets (it could be that a really really cheap token does in fact swap for 1 gwei beets in the future).

Move Treasury to use a multisig implementation

We are growing and so is our need to distribute responsibility over fund management and upgrades.

Initially the team went with a simple account owning everything for simplicity, because there was no ecosystem on BSC and little live implementations of multisig.

At this point all of those things have changed, so moving our operations to use a multisig with 4-6 community members seems like a good next step.

We can move vault upgrades and other functionality to use the same multisig. But starting with the Treasury seems good.

zapNativeToToken() test helper breaks with custom routes

At the moment zapNativeToToken() breaks when trying to handle custom routes that don't go through native.

It also can't handle acquiring LPs that are not from a Uniswap fork, for example Curve LPs

Develop generic integration tests for vaults ($1500)

Context

The initial LP strategies had unit tests for all functions and integration tests for the normal flows. As we've deployed more variations for different platforms, given the number of contracts we deploy, and how similar everything is, we've come to rely more and more on manual testing of flows.

Problem

There are some small mistakes that might go unnoticed. Some small discrepancies in pricePerFullShare for example. New entrants diminishing the share of previous users, etc that we might not catch with manual tests.

Solution

Even though the number of strats deployed keeps growing, we need to not only maintain, but increase the safety of our vaults. A great thing is that all of our vaults share the same interface, so we could have a single suite of tests that check all the usual cases. As time goes on we can build upon the starting suite.

Once we have this tests, strategists can run it on their vaults before submitting for review. Reviewers can also run it on all vaults before being displayed on the app.

Specs

Some tests that could be included:

• People can deposit and get the correct shares.
• People can withdraw and get the same value minus the withdrawal fee.
• Harvest works and all the relevant parties get rewards.
• New entrants to the vault don't negatively affect previous participants.

Some unit tests on vault's basic features would be good as well. Stress testing the shares math for example.

Gas price throttler design

To protect from front-running on harvest we gonna check transaction's gas price tx.gasprice to be less than current BSC gasprice value.
We also need to make it upgradable in case of Binance will change it as it was before from 20 to 15, and then to 10.
And to not update every single contract we can have 1 GasPrice contract deployed where other contracts will read price from.

GasPrice contract might look like this, owned by cowllector:

contract GasPrice is Ownable {

    uint public maxGasPrice = 10 gwei;   

    event NewMaxGasPrice(uint oldPrice, uint newPrice);
    
    function setMaxGasPrice(uint _maxGasPrice) external onlyOwner {
        emit NewMaxGasPrice(maxGasPrice, _maxGasPrice);
        maxGasPrice = _maxGasPrice;
    } 
}

Then we have GasThrottler contract that has gasThrottle modifier checking that tx.gasprice <= max price.
Contract which needs to limit gas will extend from it and add modifier on needed functions.

interface IGasPrice {
    function maxGasPrice() external returns (uint);
}

contract GasThrottler {
    
    address gasprice = address(0x230562106833b3618A053e6fe0bdC8fDBb59eb12);
    
    modifier gasThrottle() {
        require(tx.gasprice <= IGasPrice(gasprice).maxGasPrice(), "gas is too high!");
        _;
    }
}

I guess we can hardcode GasPrice address here and i don't see use cases when it's needed to be configurable.
We might need HecoGasThrottler, AvaxGasThrottler and so on in the future though.

And finally new Strategies will extend GasThrottler and add gasThrottle modifier on harvest().

contract StrategyLP is GasThrottler {
    
    function harvest() external gasThrottle {
        ...
    }   
}

I've deployed these 2 contracts if someone wants to test.
GasPrice without owner
https://bscscan.com/address/0x230562106833b3618A053e6fe0bdC8fDBb59eb12#code

TestStrategy with empty harvest
https://bscscan.com/address/0x5675d63494EDDa594727033864F1bca375736191#code

Make predictAddresses() work within loops

Right now we have to deploy vault/strategy pairs one at a time. This is because predictAddresses fails to predict correctly if you try to do multiple deploys in a row.

I don't exactly know why it is that it fails to predict the correct future addresses, it might be that the nonce remains cached with a call to getSigners().

If we could chain deployments, it would make it easier to launch 5-10 vaults at the same time.

Zap failed on FUSE vault

Hi!
I used to zap FUSE to ELON-FUSE LP, but now tx wants 9,5 mln gas and failed
https://explorer.fuse.io/tx/0x4817536145ba4ecdbf34ab0b42b6df31ce366f0212957b87fcfe3650b94a197e/internal-transactions
previos tx
https://explorer.fuse.io/tx/0x7bf560f77b4326f6b8385d5fc574199e4ae5244014d9a1b844027738844b38a1/token-transfers

Probably other vaults affected as well.
Plz, fix this.

Fortube harvests are failing

It seems that since yesterday all the Fortube vaults that attempted a harvest have failed. The error that the transactions get is 'Fail with error 'DEMAX PLATFORM : CHECK DGAS/TOKEN VALUE FROM FAIL'

There is almost no liquidity on any exchange for FOR, so that might be the issue

New vault: Improved auto CAKE using project pools

Hello, I want to offer myself to build a vault for cake built on top of project pools.
They have higher APRs than CAKE->CAKE so it could be a nice addition.
Do you accept strategies from new contributors?
If so I will get started but I want to make sure and not waste time

Make contract verification a part of the deploy process.

BSCScan has a verification API now, just like etherscan. It would be good to add verification into the deploy pipeline.

Create "Training Wheels Cap" contract for strategies/vaults to inherit from.

There are sometimes where we're pushing an experimental vault. Or for a few different reasons, we might want to have a temporary cap on deposits.

The goal would be to have a reusable contract that lets strategies have a cap for the first x blocks of their existence.

This would prevent people from apeing $5M into something experimental.

Beefy: Strategy Solarbeam V2 V1 ABI code is different than other ones

https://moonscan.io/address/0xeccb80e860561ca9ffb183a557f32cc90b05d3c3#readContract
This is the Beefy Strategy contract deployed on Moonbeam network and its abi is a bit different from others.

Means that you are not supporting some of the read methods from the upgraded versions.
Just want to make sure.

Create BatchFees contract

BSC is getting expensive and there's more need to optimize.

We're currently doing a bunch of swaps and ERC20 transactions with each harvest() call. We could just send it all as one to a BatchFees contract and have a public distribute() call there which would divide everything once per day for example.

This would also remove logic from the strat

Let the strat owner increase/decrease the callFee within a certain range.

We can't accurately predict which vaults are going to be the most popular. We also can't predict which vaults will be able to sustain their APYs over the long run. This creates suboptimal fees, because the callFee sometimes ends up being way too high. Beefy is paying to third parties to harvest its vaults, and sometimes it's paying way too much. This makes it so that some vaults are harvested every 2-3 minutes. That harvesting frequency doesn't provide much value to the user and it would better go somewhere else.

If we had a range of 0.05%-0.5% and we could move the callFee within that range. We could decrease how much we pay on profitable vaults, and increase it in some other vaults if it's required. It would be great if the extra % goes to the treasury for example, to help pay for more development/marketing/etc.