Summary
LibTransfer use safeTransfer without any checks.
In scenario where whitelisted by bean governance ERC20 token is upgradeable and can change safeTransfer function to malicious implementation with return bomb or just reenter funtion as callback in loop might be very dangerous for protocol.
Can lost much of funds.
Vulnerability Details
function like
withdrawDeposit
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/4e0ad0b964f74a1b4880114f4dd5b339bc69cd3e/protocol/contracts/beanstalk/silo/SiloFacet/SiloFacet.sol#L83
uses LibTransfer.sendToken
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/4e0ad0b964f74a1b4880114f4dd5b339bc69cd3e/protocol/contracts/libraries/Token/LibTransfer.sol#L68C1-L73C1
function sendToken(IERC20 token, uint256 amount, address recipient, To mode) internal {
if (amount == 0) return;
if (mode == To.INTERNAL) LibBalance.increaseInternalBalance(recipient, token, amount);
else token.safeTransfer(recipient, amount);
}
doesn't check safeTransfer
Impact
Can lost much funds and criple by reentry, siphoning or return bomb atack whole protocol.
Tools Used
slither, hardhat , code analyze
Recommendations
in LibTransfer.sol change implementation of sendToken to something like that:
1) check if success and check i there is no additional data in return or something like that data.length == 0
function sendToken(IERC20 token, uint256 amount, address recipient, To mode) internal {
if (amount == 0) return;
if (mode == To.INTERNAL) {
LibBalance.increaseInternalBalance(recipient, token, amount);
} else {
(bool success, bytes memory data) = address(token).call(
abi.encodeWithSelector(token.safeTransfer.selector, recipient, amount)
);
require(success && (data.length == 0 || abi.decode(data, (bool))), "Token transfer failed");
}
}