codehawks-bot commented 1 year ago

Missing Access Control and Front-Running Vulnerability in "sellProfits" Function

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Fees.sol#L26-L44

Summary

This audit report provides an assessment of the "sellProfits" function in the smart contract under review. It has been identified that the function lacks proper access control, making it publicly accessible. Additionally, the function contains a vulnerability where the "amountOutMinimum" parameter is hardcoded to 0. This can lead to front-running attacks, potentially causing financial losses and compromising the intended behavior of the contract.

Vulnerability Details

The "sellProfits" function allows anyone to call it, as there is no access control mechanism in place. This lack of access control means that any external address can execute the function, potentially leading to unauthorized token swaps.

Additionally, the function employs the "swapRouter.exactInputSingle" method with the "amountOutMinimum" parameter hardcoded to 0. By setting "amountOutMinimum" to 0, the contract becomes vulnerable to front-running attacks, where malicious actors can monitor transactions, identify profitable swaps, and execute their own transactions before the original transaction is confirmed. This can result in the contract receiving less valuable tokens than expected, leading to potential financial losses. With flashswap thsi can be return zero tokens as result of exchange

Impact

The absence of proper access control in the "sellProfits" function allows any external address to call it, potentially leading to unauthorized token swaps and misuse of the contract's functionality.

Furthermore, the hardcoded "amountOutMinimum" set to 0 makes the contract susceptible to front-running attacks. This can result in financial losses and may undermine the intended operation of the contract.

Tools Used

VS Code

Recommendations

To address the identified vulnerabilities, the following recommendations are proposed:

1.Implement Access Control: Add appropriate access control mechanisms to restrict the execution of the "sellProfits" function to authorized addresses only. This can be achieved through the use of modifiers or other access control patterns, such as OpenZeppelin's "Ownable" or "AccessControl" contract.

2.Dynamic Calculation of "amountOutMinimum": Instead of hardcoding the "amountOutMinimum" to 0, calculate an accurate value based on the expected slippage and market conditions. This will help ensure that token swaps are executed within acceptable parameters and prevent front-running attacks.

B353N commented 1 year ago

[ESCALATION] I think issue is difrent from others. Root case is missing access control and then lag of amountOutMinimum because this miss is possible to make this attack true. Because as we can see in recomendation of selected report: https://github.com/Cyfrin/2023-07-beedle/issues/1247

-   function sellProfits(address _profits) public {
+   function sellProfits(address _profits, uint256 _amountOutMinimum, uint160 _sqrtPriceLimitX96) public {
        require(_profits != WETH, "not allowed");
        uint256 amount = IERC20(_profits).balanceOf(address(this));

        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter
            .ExactInputSingleParams({
                tokenIn: _profits,
                tokenOut: WETH,
                fee: 3000,
                recipient: address(this),
                deadline: block.timestamp,
                amountIn: amount,
-               amountOutMinimum: 0,
-               sqrtPriceLimitX96: 0
+               amountOutMinimum: _amountOutMinimum,
+               sqrtPriceLimitX96: _sqrtPriceLimitX96
            });

        amount = swapRouter.exactInputSingle(params);
        IERC20(WETH).transfer(staking, IERC20(WETH).balanceOf(address(this)));
    }

He recomendate to pass the amountOutMinimum and sqrtPriceLimitX96 as dinamyc imput but if there no have modifier for onlyOwner everybody can call the function and pass again amoutOutMinimum to 0 and make sandwitch attack.

So issue is for missing access control of sellProfit function, because if there have access control modifier and then make amoutOutMinimum dinamyc then this sandwitch attack can't happening.
I think issue is new one funding-accesscontrol-sellprofit-mev or something like that

kosedogus commented 1 year ago

While judging above escalation, would recommend checking discussion in #393.

Cyfrin / 2023-07-beedle