Unrestricted Withdraw Function in Silo.sol

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/main/src/Silo.sol#L28-L30

Vulnerability details

Impact

The function "withdraw" is used to move funds from the contract to a specified address. It is only supposed to be accessed by the STAKING_VAULT, as per the "onlyStakingVault" modifier.

However, there is no check incorporated to ensure that the "to" address is not a malicious one or an attacker's address. This could allow an attacker to drain funds from the contract.

Proof of Concept

https://github.com/code-423n4/2024-07-loopfi/blob/main/src/Silo.sol#L28-L30

function withdraw(address to, uint256 amount) external onlyStakingVault {
    lpETH.safeTransfer(to, amount);
}

The 'to' parameter in the function can lead to potential security risks.

Tools Used

Manual Review

Recommended Mitigation Steps

A solution to this problem could be to implement a whitelist of addresses that are authorized to receive transfers from the contract. It could also be beneficial to include a capability to update the whitelist. After implementing this, the withdraw function should check if the 'to' address is within the whitelist before executing the transfer. Here is an example of how to do this:

mapping (address => bool) private whitelist;
    
 modifier onlyWhitelisted(address _address) {
    require(whitelist[_address] == true, "Not whitelisted");
    _;
 }

function setWhitelist(address _address, bool _state) external onlyOwner {
    whitelist[_address] = _state;
}

function withdraw(address to, uint256 amount) external onlyStakingVault onlyWhitelisted(to) {
    lpETH.safeTransfer(to, amount);
}

In this example, only the contract owner can set the whitelist state of an address, and the 'withdraw' function now has an extra 'onlyWhitelisted' modifier, which ensures that the 'to' address is in the whitelist. This serves to enhance the smart contract's security.

Assessed type

Rug-Pull