Fees.sol - sellProfits() - Open function - Anyone can swap contracts balance to any token. Anyone can sell tokens

Severity

Medium Risk

Relevant GitHub Links

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

Summary

The Fees contract in the provided Solidity code snippet contains an open access vulnerability in the sellProfits method, which could potentially allow any user or contract to swap the contract's balance into any desired token.

Vulnerability Details

The sellProfits method is set as public and without any restrictions or access control mechanisms. This means that any external actor (individuals, contracts) can call this method and initiate a token swap, converting all available _profits tokens to WETH, which could result in unauthorized and possibly malicious transactions.

function sellProfits(address _profits) 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
        });

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

This method, when called, swaps the _profits token for WETH (Wrapped Ether). The code lacks any mechanism to prevent unauthorized users from executing this function, resulting in a potential vulnerability.

Impact

If exploited, this vulnerability can lead to unauthorized and potentially malicious token swaps, it could allow an attacker to manipulate the contract for their own advantage.

Tools Used

Manual review.

Recommendations

It is highly recommended to incorporate access control mechanisms into the sellProfits method. The OpenZeppelin library provides simple and effective solutions for role-based access control. A modifier that checks if the caller has the appropriate role before executing the function could prevent unauthorized usage. Implementing an onlyOwner or onlyAuthorized modifier could be an effective way to restrict access to this function. Here is a simple example:

modifier onlyOwner {
    require(msg.sender == owner, "Caller is not the owner");
    _;
}

function sellProfits(address _profits) public onlyOwner {
    // existing code
}

The onlyOwner modifier checks that the sender of the transaction is the owner before the execution of the function continues. The owner address could be set at the contract's deployment.

Please note that this is a simple solution and your exact implementation might require more complex access control. For instance, you might want to use OpenZeppelin's AccessControl contract to create more granular roles and permissions.

Cyfrin / 2023-07-beedle