ConvertSilo Manual Review Findings

CSO-01M: Potential Re-Entrancy Attack Vector

Type	Severity	Location
External Call Validation		ConvertSilo.sol:L97

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:

Copy
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

Type	Severity	Location
Logical Fault		ConvertSilo.sol:L133

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:

Copy
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.