Beanstalk: The Finale
Beanstalk
DeFiHardhatFoundry
250,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Protocol doesn't consider the tractor logic with the implementation of the `LibEth#refundEth()`

bauchibred

Summary

The implementation of LibEth#refundEth() does not account for the new Tractor logic, leading to issues in refunding ETH to the correct user, since refundEth() always uses msg.sender directly instead of querying the user via LibTractor_user(). As a result, ETH refunds might be sent to the wrong address when executed through the Tractor.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/beanstalk/migration/L1Libraries/LibEth.sol#L15-L23

library LibEth {
function refundEth() internal {
AppStorage storage s = LibAppStorage.diamondStorage();
if (address(this).balance > 0 && s.isFarm != 2) {
(bool success, ) = msg.sender.call{value: address(this).balance}(new bytes(0));
require(success, "Eth transfer Failed.");
}
}
}

This library ends up being queried in multiple instances and it's use is to refund Eth in the case where there is a need to do so, see instances where it's being used:

​
protocol/contracts/beanstalk/farm/DepotFacet.sol:
55 results = IPipeline(PIPELINE).advancedPipe{value: value}(pipes);
56: LibEth.refundEth();
57 }
​
69 result = IPipeline(PIPELINE).pipe{value: value}(p);
70: LibEth.refundEth();
71 }
​
protocol/contracts/beanstalk/farm/FarmFacet.sol:
59 s.sys.isFarm = 1;
60: LibEth.refundEth();
61 }
​
protocol/contracts/beanstalk/farm/TokenFacet.sol:
226 LibWeth.wrap(amount, mode);
227: LibEth.refundEth();
228 }
​
protocol/contracts/beanstalk/migration/L1TokenFacet.sol:
195 LibWeth.wrap(amount, mode);
196: LibEth.refundEth();
197 }

Evidently, this functionality is rampant accross different facets, but the latest update with Beanstalk, includes the Tractor logic.

Now, from the walkthrough videos and the documentation, we can see that the intended functionality is to have the Tractor be able to be substituted by users for any of their logic, the previous sentence is probably not worded correctly, but the idea is that users can now pass most of their operations via the tractor.

Which is why if you go to the PR with the changes from the previous version to the current one here: https://github.com/BeanstalkFarms/Beanstalk/pull/909/files?file-filters%5B%5D=.md&file-filters%5B%5D=.sol&show-deleted-files=true&show-viewed-files=true, we'd notice most instances of msg.sender being implemented previously have now been changed to query the user via LibTractor_user() i.e https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/libraries/LibTractor.sol#L116-L122

function _user() internal view returns (address payable user) {
user = _getActivePublisher();
if (uint160(bytes20(address(user))) <= 1) {
user = payable(msg.sender);
}
}
​

However this has not been considered in LibEth as shown above in it's refundETH() implementation, which then causes that in instances where LibEth#refundETH() in all the facets where it's being used if the execution is via the Tractor, then the attempt to refund the ETH would be done on the wrong address.

NB: This bug case is somewhat applicable in different context to other instances where after the Tractor update, msg.sender is still being implemented instead of the LibTractor_user().

Impact

When using the new tractor feature and there is a need to refund Eth, end users lose out on their tokens cause the refund is not done to the correct end user address.

Tools Used

Manual review

Recommendations

Consider refactoring LibEth#refundETH() to take into account the fact that it could be called in the context where the execution is from the Tractor specification.

Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Refunding ETH to the caller can be exploited using the Tractor component to call any arbitrary function on behalf of the publisher

Appeal created

giovannidisiena Auditor
12 months ago
inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.