LibMarket Static Analysis Findings

LMT-01S: Potentially Dangerous Refund Scheme

Type	Severity	Location
Logical Fault		LibMarket.sol:L57, L63, L88, L123, L183, L214

Description:

The LibMarket contract has been built with user refunds in mind, transferring native currency to the user in case of leftover funds. While a desired trait of the system, it can compromise security assumptiosn of the system such as those by LibCheck and in particular the bean balance check. As the call is re-entrant during a refund, the main Beanstalk contract may be possessing Bean units that will otherwise be consumed during the outer call and the re-entrant attacker can exploit this to provide a superficial balanceOf evaluation in the LibCheck contract and bypass it.

Example:

Copy
55function buy(uint256 buyBeanAmount) internal returns (uint256 amount) {
56    (uint256 ethAmount, uint256 beanAmount) = _buy(buyBeanAmount, msg.value, msg.sender);
57    (bool success,) = msg.sender.call{ value: msg.value.sub(ethAmount) }("");
58    require(success, "Market: Refund failed.");
59    return beanAmount;
60}

Recommendation:

We advise the system to be redesigned to follow a pull-over-push paradigm whereby refunds are instead accredited to the user who is then responsible to claim them via a simple function.

Alleviation:

The code has been significantly refactored to apply a refund at the end of each function's execution preventing re-entrancies from occuring mid-call. Additionally, the non-reentrant modifier has been introduced throughout the codebase thereby alleviating this exhibit in full.