Omniscia Beanstalk Audit

ConvertSilo Manual Review Findings

ConvertSilo Manual Review Findings

CSO-01M: Potential Re-Entrancy Attack Vector

Description:

The _convertAddAndDepositLP relies on a balanceOf evaluation to check whether a transfer from the user is required, a check that can be bypassed by performing one of the market operations all of which are re-entrant due to their ETH refund operations.

Example:

protocol/contracts/farm/facets/ConvertFacet/ConvertSilo.sol
97require(id <= season(), "Silo: Future crate.");
98(uint256 crateAmount, uint256 crateBase) = lpDeposit(account, id);
99require(crateAmount >= amount, "Silo: Crate balance too low.");
100require(crateAmount > 0, "Silo: Crate empty.");

Recommendation:

We advise this check to be guarded against properly by enforcing a global non-reentrant flag across the codebase, potentially at the Diamond proxy level, to ensure this behaviour cannot be exploited. Alternatively, we advise all ETH refund operations in the LibMarket contract to utilise a pull pattern instead, preventing re-entrancies altogether.

Alleviation:

The relevant function was relocated to the LibLPSilo function, however, the refund system of Beanstalk was updated to instead pay out refunds at the end of each call's execution thereby preventing mid-call re-entrancies. Additionally, functions that invoke this function implementaiton are themselves marked as non-reentrant thereby alleviating this exhibit.

CSO-02M: Inconsistent Balance Check

Description:

The _depositBeans function of the ConvertSilo enforces the beanBalanceCheck of LibCheck, however, the BeanSilo implementation does not. In contrast, the _withdrawBeansForConvert function of ConvertSilo does not impose the check whereas the _withdrawBeans function does so.

Example:

protocol/contracts/farm/facets/ConvertFacet/ConvertSilo.sol
125function _depositBeans(uint256 amount, uint32 _s) internal {
126 require(amount > 0, "Silo: No beans.");
127 incrementDepositedBeans(amount);
128 uint256 stalk = amount.mul(C.getStalkPerBean());
129 uint256 seeds = amount.mul(C.getSeedsPerBean());
130 if (_s < season()) stalk = stalk.add(stalkReward(seeds, season()-_s));
131 depositSiloAssets(msg.sender, seeds, stalk);
132 addBeanDeposit(msg.sender, _s, amount);
133 LibCheck.beanBalanceCheck();
134}

Recommendation:

We advise behaviour to be standardised across the contracts depending on the desired behaviour.

Alleviation:

After consideration, the Beanstalk team identified these invocations as redundant and instead utilizes the balanceCheck function on three places within the codebase where it is deemed necessary. As a result, we consider this exhibit nullified.