Direct theft of any user funds, whether at-rest or in-motion, other than unclaimed yield
Bug Description
This report outlines a critical vulnerability identified in the well implementation contract. The vulnerability enables an attacker to drain all excess tokens from the well contract, leading to loss of excess funds and funds mistakenly sent to the well contract. This enables the attacker to cause significant risks to the integrity and functionality of the protocol.
The attacker can exploit this vulnerability by calling the skim() function which doesn't only return an array of excess tokens but also transfers the tokens to the caller. The attacker is able to take advantage of the flaw in this contract because the skim() function utilized in the recovery of excess tokens is an external function which has no access control and can be invoked by anyone.
Therefore i) excess tokens ii) large tokens sent directly to the well contract in error by the protocol or users can be stolen by malicious attackers repeatedly without mitigation.
Impact
Critical has large volume of funds sent in error by protocol or user to the well contract can siphoned by attacker.
The stealing of excess funds can occur repeatedly under the nose of the protocol.
Risk Breakdown
Difficulty to Exploit: Easy
Weakness: High
CVSS2 Score: 9
Recommendation
Access control modifier or require statement must be added to skim() function whereby excess tokens can only be withdrawn by admin or governance.
//mitigation by adding admin modifier
function skim(address recipient) external onlyRole(Admin) nonReentrant returns (uint256[] memory skimAmounts){
require(isAdmin(msg.sender), "must have admin privileges");
}
Step a- A User named bob sends tokens directly to the well contract or the beanstalk protocol itself directly transfers large volumes of tokens to the well contract by error.
Step b- An attacker calls the skim() function by passing their own address as recipient and drains the all the excess tokens in the well contract of the beanstalk protocol.
** Attacker can exploit this vulnerability perpetually under the nose of the protocol and continues to siphon excess tokens in the well contract.
Below is a POC to prove that malicious actor can siphon the excess funds available in the well contract.
BIC Response
This is not a valid bug report because it describes expected behavior. As explained in the PoC:
Step a- A User named bob sends tokens directly to the well contract or the beanstalk protocol itself directly transfers large volumes of tokens to the well contract by error.
It is intended functionality that skim can remove excess tokens sent to the Well.
Due to these reasons, we are closing the submission and no reward will be issued.
function test_skim() public prank(user) {
vm.assume(amounts[0] <= 800e18);
vm.assume(amounts[1] <= 800e18);
// Transfer from user Test contract to Well
tokens[0].transfer(address(well), amounts[0]);
tokens[1].transfer(address(well), amounts[1]);
Balances memory wellBalanceBeforeSkim = getBalances(
address(well),
well
);
// Verify that the Well has received the tokens
assertEq(wellBalanceBeforeSkim.tokens[0], 1000e18 + amounts[0]);
assertEq(wellBalanceBeforeSkim.tokens[1], 1000e18 + amounts[1]);
// Get a user with a fresh address (no ERC20 tokens)
address _user = users.getNextUserAddress();
uint256[] memory reserves = new uint256[](2);
// Verify that the user has no tokens
Balances memory userBalanceBeforeSkim = getBalances(_user, well);
reserves[0] = userBalanceBeforeSkim.tokens[0];
reserves[1] = userBalanceBeforeSkim.tokens[1];
assertEq(reserves[0], 0);
assertEq(reserves[1], 0);
well.skim(_user);
Balances memory userBalanceAfterSkim = getBalances(_user, well);
Balances memory wellBalanceAfterSkim = getBalances(address(well), well);
// Since only 1000e18 of each token was added as liquidity, the Well's reserve
// should be reset back to this.
assertEq(wellBalanceAfterSkim.tokens[0], 1000e18);
assertEq(wellBalanceAfterSkim.tokens[1], 1000e18);
// The difference has been sent to _user.
assertEq(
userBalanceAfterSkim.tokens[0],
amounts[0],
"token 0 balance increase"
);
assertEq(
userBalanceAfterSkim.tokens[1],
amounts[1],
"token 1 balance increase"
);
}