Ceres Farm Contracts

Description

According to the vault strategy contract comment guide

/**     * @dev Optional function for strategist to override that can     *  be called in between reports.     *     * If '_tend' is used tendTrigger() will also need to be overridden.     *     * This call can only be called by a permissioned role so may be     * through protected relays.     *     * This can be used to harvest and compound rewards, deposit idle funds,     * perform needed position maintenance or anything else that doesn't need     * a full report for.     *     *   EX: A strategy that can not deposit funds without getting     *       sandwiched can use the tend when a certain threshold     *       of idle to totalAssets has been reached.     *     * This will have no effect on PPS of the strategy till report() is called.     *     * @param _totalIdle The current amount of idle funds that are available to deploy.     */    function _tend(uint256 _totalIdle) internal virtual {}
    /**     * @dev Optional trigger to override if tend() will be used by the strategy.     * This must be implemented if the strategy hopes to invoke _tend().     *     * @return . Should return true if tend() should be called by keeper or false if not.     */    function _tendTrigger() internal view virtual returns (bool) {        return false;    }

The _tendTrigger return True if there are idle asset but does not handle the idle asset,

so if a off-chain keeper that relies on the _tendTrigger() to give right signal can keep trying to trigger tend rebalance even the tvl is not out of range.

Recommendation

function _tendTrigger() internal view override returns (bool) {        // _tend needs to be triggered if the strategy LTV is outside configured range        return _isLtvOutOfRange();    }

If the intention is only to trigger rebalance when ltv is out of range, then only return True if ltv is out of range.

The keepers can always call rebalanceStrategy to trigger idle asset deposit.

Description

CeresLeveragedStrategy inherits BaseStrategy which delegates calls to TokenizedStrategy. The latter uses storage based reentrancy protection mechanism based on the storage variable of S.entered while CeresLeveragedStrategy relies on ReentrancyGuardTransient.

We want to block any potential reentrancy between any given pair of functions of the deployed contract and currently it is theoretically possible for cross function reentrancies to occur between state changing functions from CeresLeveragedStrategy and TokenizedStrategy.

As a concrete example, let's take the scenario of rebalanceStrategy() being called, this function is calling many external contracts, in case one of these calls can call back, it might call a function of TokenizedStrategy (like withdraw() for instance) via the BaseStrategy fallback function and might manipulate the CeresLeveragedStrategy contract by doing so.

Recommendation

Consider changing CeresLeveragedStrategy to use the storage based reentrancy protection mechanism that's already used by TokenizedStrategy instead of the current one which is based on transient storage.

Cantina

Fixed by implementing the reviewer's recommendation.

Description

newStrategy() allows anyone to initialize a new Strategy contract, then the newly deployed address is stored in deployments:

deployments[_asset] = address(_newStrategy);

The issue here is that anyone can call this function for a given asset with a different strategy and it will over-write the value stored for this asset inside deployments.

Recommendation

Consider restricting access to this function to privileged users only.

Cantina

Fixed by implementing the reviewer's recommendation.

Description

https://silodocs2.netlify.app/docs/users/using-silo/supply#withdrawing

the code may withdraw collateral from silo during rebalance or emergency withdraw.

However, when the collateral are borrowed out in silo, the withdraw fails when utilization rate is high

Suppliers may withdraw their deposit, including accrued interest, at any time. The exception is when liquidity is fully utilized where interest rates are expected to be high to encourage repayments and attract new deposits.

Recommendation

Add code to monitor utilization rate and if utilization rate is high, skip withdraw collateral from Silo to avoid transaction revert.

Description

function setTargetLtv(uint256 _targetLtv) external onlyManagement {    // Target LTV should be less than maxLtv allowed    uint256 maxLtv = getMaxLtvSilo();    if (_targetLtv + LTV_BUFFER > maxLtv) revert LibError.AboveMaxValue(maxLtv);    configTargetLtv = _targetLtv;    emit TargetLtvSet(_targetLtv);}

As we can see, configTargetLtv is not explicitly checked to be less than WAD, instead the check relies on getMaxLtvSilo() max value to be WAD (10^18).

Later in the code, we have two instances of subtraction that will cause a revert in case configTargetLtv >= WAD:

uint256 maxLoan = (totalAssetsInBorrowToken * configTargetLtv) / (WAD - configTargetLtv);

uint256 targetDebt = (_convertAssetToBorrowToken(totalAssets - _amount) * configTargetLtv) /    (WAD - configTargetLtv);

Recommendation

For better code readability and no external reliance in case Silo updates their code, consider adding an explicit check in setTargetLtv() to make sure configTargetLtv is less than WAD.

Cantina

Fixed by implementing the reviewer's recommendation.

Description

CeresSwapper is a non-upgradeable contract used by CeresLeveragedStrategy as a wrapper to support token swapping functionality. CeresSwapper is not meant to serve other accounts/contracts besides CeresLeveragedStrategy and the main reason behind deploying it as a separate contract (instead of a library) is to maintain future flexibility to replace it in the future with a new version. Having the CeresSwapper main functionality callable by any address increases attack surface without any significant upside in contrast to the well known principal of least privileges better described in least-privilege.md.

Recommendation

Consider restricting access to the main state changing functions of CeresSwapper: swapFrom, swapUsingAggregator, swapTo to CeresLeveragedStrategy only. Noting that it will make a circular dependency issue around initialization, it will require having an initialize function in the CeresSwapper that will store the address of the CeresLeveragedStrategy contract.

Cantina

Fixed by implementing the reviewer's recommendation.

Description

The following functions return values that are not currently used:

1. _pendleSwapTokenForPt()
2. _pendleSwapPtForToken()
3. _algebraV4SwapExactIn()

Recommendation

Consider removing these return values from these functions.

Cantina

Fixed by implementing the reviewer's recommendation.

Description

The following functions are missing event emissions:

1. swapFrom()
2. swapUsingAggregator()
3. swapTo()
4. setSwapPath()
5. setSwapProvider()
6. setSwapAggregator()

Recommendation

Consider adding events to reflect state changes.

Cantina

Fixed by implementing the reviewer's recommendation.

Description

Both _kyberCachedSwap() and _kyberAggregatorSwap() use a storage address (router) for low level external calls. The caller of the two functions can specify any function in the router contract. Although it is not a fully arbitrary call, we still think this ability should be limited to reduce surface area.

if (canBeScaled) {    (bool isSuccess, ) = router.call(updatedSwapData);    require(isSuccess, LibError.SwapFailed());} else {    revert LibError.ScaledInputFailed();}

Recommendation

Consider replacing the arbitrary call with the actual interface function(s) call of the Kyber contract, this restriction will restrict the flexibility of the CeresSwapper contract which is not upgradeable, but since this contract should not hold any tokens post transaction, and can be set in the strategy contract, you can easily replace it with a new one in case changes are needed.

Description

swapFrom(), swapUsingAggregator(), swapTo() are missing reentrancy protection, although calls to external contracts are being made inside each of these functions.

Recommendation

To reduce surface area, consider implementing reentrancy guards in the functions mentioned above.

Cantina

Fixed by implementing the reviewer's recommendation.

Description

1. CeresLeveragedStrategy: PENDLE_ROUTER is an immutable variable that is initialized but not used in the contract.
2. CeresSwapper: swapTo() is missing Natspec documentation.

Recommendation

1. Consider removing PENDLE_ROUTER from declaration and assignment in the constructor of CeresLeveragedStrategy.
2. Add Natspec documentation to swapTo().

Cantina

Fixed by implementing the reviewer's recommendation.

Description

The constructor of CeresLeveragedStrategy is missing the initialization of ceresSwapper which is supposed to happen only later inside setSwapper(). In case the deployer forgets to call setSwapper() right after deployment, many code paths in the contract will revert, mainly rebalanceStrategy() which will delay potential earning of yield for depositors.

Recommendation

Consider setting the value of ceresSwapper in the constructor.

Cantina

Fixed by implementing the reviewer's recommendation.

Description

Both _algebraV4SwapExactIn() and _algebraV4SwapExactOut() read twice from storage although the value is already stored in swapPath as we can see:

bytes memory swapPath = tokenSwapPath[tokenPairHash][SwapType.ALGEBRA_V4_SWAPX];if (swapPath.length > 0) {    // Use the stored swap path for the token pair    IV3SwapRouter.ExactOutputParams memory exactOutputParams = IV3SwapRouter.ExactOutputParams({        path: tokenSwapPath[tokenPairHash][SwapType.ALGEBRA_V4_SWAPX],        recipient: address(this),        amountOut: amountOut,        amountInMaximum: maxAmountIn    });    actualAmountIn = swapRouter.exactOutput(exactOutputParams);} else {

Recommendation

Consider using the value of the cached swapPath instead.

Cantina

Fixed by implementing the reviewer's recommendation.