LidoVault contract has Lack of Access Control on the getFixedOngoingWithdrawalRequestIds Function

Sherlock.xyz Security Audit Findings Report

Project: LidoVault Contract

Date: September 2024

Audited by: fat32

Summary

During the audit of the LidoVault smart contract, a critical access control vulnerability was identified. The getFixedOngoingWithdrawalRequestIds() function is publicly accessible and lacks access control, allowing any external user to retrieve the withdrawal request IDs of any user. This exposes sensitive data and opens the possibility for malicious actors to obtain private information about other users' vault interactions, which could lead to more targeted or sophisticated attacks.

This audit report presents the vulnerability, a proof of concept (PoC) to demonstrate the issue, suggested mitigations, and improvements.

Lack of Access Control on the `getFixedOngoingWithdrawalRequestIds` Function

Severity: High

Impact

The getFixedOngoingWithdrawalRequestIds() function is vulnerable due to the absence of access control, enabling any external user to query the ongoing withdrawal request IDs for any user in the vault. This can lead to sensitive data exposure, including revealing other users' withdrawal patterns. An attacker can gather private information about vault users, which may expose them to more sophisticated attacks such as phishing or front-running based on known withdrawal patterns.

Vulnerable Code Location

URL: Vulnerable Code on GitHub

function getFixedOngoingWithdrawalRequestIds(address user) public view returns (uint256[] memory) {
return fixedToVaultOngoingWithdrawalRequestIds[user].requestIds;
}

Proof of Concept (PoC)

This PoC demonstrates how an unauthorized user can call the getFixedOngoingWithdrawalRequestIds() function to retrieve the withdrawal request IDs of another user without permission.

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

import {Test, console} from "forge-std/Test.sol";
import {LidoVault} from "../contracts/LidoVault.sol";

import "../contracts/interfaces/ILidoVault.sol";
import "../contracts/interfaces/ILido.sol";
import "../contracts/interfaces/ILidoWithdrawalQueueERC721.sol";
import "../contracts/interfaces/ILidoVaultInitializer.sol";

contract LidoVaultAccessControlTest is Test {
    LidoVault public lidovault;
    address public account;
    ILido public ilido;
    ILidoVault public ilidovault;
    ILidoWithdrawalQueueERC721 public ilidowithdrawal;
    ILidoVaultInitializer public ilidovaultinitializer;
    ILidoVaultInitializer.InitializationParams public initializationparams;

    // Mock Lido contract
    function setUp() public {
        account = address(this);
        uint256 newBalance = 100 ether;
        lidovault = new LidoVault(true); // Initializing LidoVault with a true parameter
        ilido = ILido(address(this)); // Deploying a mock Lido contract
        vm.deal(account, newBalance); // Deal ether to the test account
    }

    function testAccessController() external {
        uint256 amount = 1000;
        address attacker = address(0xbEEF);
        address user = address(1157920892373111111111111111111111111111111111111);
        vm.startPrank(attacker);
        //lidovault.initialize(ILidoVaultInitializer.InitializationParams({ vaultId: type(uint256).max, duration: type(uint256).max, fixedSideCapacity: type(uint256).max, variableSideCapacity: type(uint256).max, earlyExitFeeBps: type(uint256).max, protocolFeeBps: type(uint256).max, protocolFeeReceiver: address(0xbEEF) }));
        lidovault.getFixedOngoingWithdrawalRequestIds(account);
        vm.stopPrank();
    }
}

Foundry Log Results

lido-fiv % forge test -vvvv --match-contract LidoVaultAccessControl
[⠊] Compiling...
...
Ran 1 test for test/LidoVaultAccessControl.t.sol:LidoVaultAccessControlTest
[PASS] testAccessController() (gas: 13817)
Traces:
  [13817] LidoVaultAccessControlTest::testAccessController()
    ├─ [0] VM::startPrank(0x000000000000000000000000000000000000bEEF)
    │   └─ ← [Return] 
    ├─ [2987] LidoVault::getFixedOngoingWithdrawalRequestIds(LidoVaultAccessControlTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]) [staticcall]
    │   └─ ← [Return] []
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    └─ ← [Stop] 

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.46ms (301.46µs CPU time)

Ran 1 test suite in 121.25ms (1.46ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

This Foundry test logs confirm that the function can be called successfully by an unauthorized address (0xBEEF), indicating a lack of access control on the getFixedOngoingWithdrawalRequestIds() function. The test passes, meaning that unauthorized users can retrieve sensitive information from the vault, potentially leading to exploitation.

Mitigation

To prevent unauthorized access, restrict access to the getFixedOngoingWithdrawalRequestIds() function by implementing an access control mechanism such as onlyDepositor or onlyOwner. This will ensure that only users with an active deposit or the contract owner can call the function.

Here’s a mitigation using a modifier to restrict access to legitimate depositors:

// Access Control Modifier
modifier onlyDepositor() {
    require(fixedETHDepositToken[msg.sender] > 0 || variableBearerToken[msg.sender] > 0, "Not a depositor");
    _;
}

function getFixedOngoingWithdrawalRequestIds(address user) public view onlyDepositor returns (uint256[] memory) {
    return fixedToVaultOngoingWithdrawalRequestIds[user].requestIds;
}

Explanation:

onlyDepositor modifier: Ensures that only users who have made a deposit (either to the fixed or variable side) can access sensitive information like ongoing withdrawal request IDs.
Updated function: The getFixedOngoingWithdrawalRequestIds() function now includes the onlyDepositor modifier to restrict access to depositors only, preventing unauthorized users from querying sensitive data.

This solution ensures that only legitimate users with deposits can access their withdrawal request IDs, preventing malicious actors from exploiting the function.

Conclusion

Summary of Findings:

Lack of Access Control on Sensitive Functions: Unauthorized users can access sensitive information, such as ongoing withdrawal request IDs, through the getFixedOngoingWithdrawalRequestIds() function.
- Severity: High
- Type: Access Control Vulnerability
- Mitigation: Implement access control using the onlyDepositor modifier to restrict access to depositors.

By implementing the recommended access control mechanisms, the LidoVault contract will be more secure against unauthorized access to sensitive user information and will safeguard user data integrity.

sherlock-audit / 2024-08-saffron-finance-judging

fat32 - LidoVault contract has Lack of Access Control on the getFixedOngoingWithdrawalRequestIds Function #115