Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In tailOff() of BathPiar users funds will be lost if strategist call function with wrong parameters #247

Closed
code423n4 opened this issue May 28, 2022 · 3 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L533-L563

Vulnerability details

Impact

Function tailOff() is callable by strategist and it sends BathToken funds to _stratUtil address. There is no whitelist check for the destination address and if strategist by mistake or intentionally call tailOff() by wrong values liquidity providers funds will be lost.

Proof of Concept

This is tailOff() code in BathPair:

    /// @notice Function to attempt inventory risk tail off on an AMM
    /// @dev This function calls the strategist utility which handles the trade and returns funds to LPs
    function tailOff(
        address targetPool,
        address tokenToHandle,
        address targetToken,
        address _stratUtil, // delegatecall target
        uint256 amount, //fill amount to handle
        uint256 hurdle, //must clear this on tail off
        uint24 _poolFee
    ) external onlyApprovedStrategist(msg.sender) {
        // transfer here
        uint16 stratRewardBPS = IBathHouse(bathHouse).getBPSToStrats();

        IBathToken(targetPool).rebalance(
            _stratUtil,
            tokenToHandle,
            stratRewardBPS,
            amount
        );

        // Should always exceed hurdle given amountOutMinimum
        IStrategistUtility(_stratUtil).UNIdump(
            amount.sub((stratRewardBPS.mul(amount)).div(10000)),
            tokenToHandle,
            targetToken,
            hurdle,
            _poolFee,
            targetPool
        );
    }

As you can see it calls rebalance() of targetPool and it will send amount underlying token to _stratUtil address. if strategist set _stratUtil value wrongly then users funds will be lost forever.
There should be some whitelist for _stratUtil that strategist can only send funds to them, is not secure to totally deponed on strategists not making mistake or not doing anything wrong.

Tools Used

VIM

Recommended Mitigation Steps

add _stratUtil addresses list in BathPair and confirm them.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels May 28, 2022
code423n4 added a commit that referenced this issue May 28, 2022
@KenzoAgada
Copy link

Feels to me more like a best practice recommendation than a real vulnerability. Anyway high severity is too high.

@bghughes bghughes added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Jun 7, 2022
@bghughes
Copy link
Collaborator

bghughes commented Jun 7, 2022

This function is with onlyApprovedStrategist which are centralized/controlled EOAs only. See #344 #133

@HickupHH3
Copy link
Collaborator

a weak duplicate of #211 because it's about incorrect parameters as opposed to malicious parameters. I'll mark it as such nonetheless.

@HickupHH3 HickupHH3 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Jun 21, 2022
@HickupHH3 HickupHH3 added the duplicate This issue or pull request already exists label Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

4 participants