Summary

no check on transfer.

Vulnerability Details

https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/main/protocol/contracts/libraries/Convert/LibPipelineConvert.sol#L53

function executePipelineConvert(
address inputToken,
address outputToken,
uint256 fromAmount,
uint256 fromBdv,
uint256 initialGrownStalk,
AdvancedFarmCall[] calldata advancedFarmCalls
) external returns (uint256 toAmount, uint256 newGrownStalk, uint256 newBdv) {
PipelineConvertData memory pipeData = LibPipelineConvert.populatePipelineConvertData(
inputToken,
outputToken
);

Impact

Some major tokens went live before ERC20 was finalised, resulting in a discrepancy whether the transfer functions a) should return a boolean or b) revert/fail on error. The current best practice is that they should revert, but return “true” on success. However, not every token claiming ERC20-compatibility is doing this — some only return true/false; some revert, but do not return anything on success. This is a well known issue, heavily discussed since mid-2018.

Today many tools, including OpenZeppelin, offer a wrapper for “safe ERC20 transfer”: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol

Tools Used

Recommendations

1. Use something like OpenZeppelin’s SafeERC20

2. Set up an allow list for tokens, which are knowingly safe