Squilliam - [M-2] Locked Ether Vulnerability in `TitlesGraph` Contract (Payable Functions without Withdrawal + Funds Permanently Locked)

[sherlock-admin4]

Squilliam

medium

[M-2] Locked Ether Vulnerability in TitlesGraph Contract (Payable Functions without Withdrawal + Funds Permanently Locked)

Summary

The TitlesGraph contract has two functions marked as payable, allowing the contract to receive ether. However, the contract does not have a mechanism to withdraw the received ether, resulting in the funds being permanently locked in the contract.

Vulnerability Detail

Add this code to TitlesGraph.t.sol

function test_lockedEther() public {
    // Get the initial balance of the TitlesGraph contract
    uint256 initialBalance = address(titlesGraph).balance;

    // Send some ether to the grantRoles function
    address payable titlesGraphPayable = payable(address(titlesGraph));
    titlesGraph.grantRoles{value: 1 ether}(address(0), 0);

    // Check that the contract's balance has increased
    assertEq(address(titlesGraph).balance, initialBalance + 1 ether);

    // Try to withdraw the ether (this should fail)
    vm.expectRevert();
    titlesGraphPayable.transfer(1 ether);
}

The test above proves the vulnerability by performing the following steps:

1. It retrieves the initial balance of the TitlesGraph contract using address(titlesGraph).balance and stores it in the initialBalance variable.
2. It sends 1 ether to the grantRoles function of the TitlesGraph contract using titlesGraph.grantRoles{value: 1 ether}(address(0), 0). The {value: 1 ether} syntax specifies the amount of ether to send along with the function call.
3. It verifies that the contract's balance has increased by 1 ether by comparing address(titlesGraph).balance with initialBalance + 1 ether using the assertEq statement.
4. It attempts to withdraw the ether from the contract by calling titlesGraphPayable.transfer(1 ether). However, since there is no withdrawal function in the TitlesGraph contract, this call fails, and the test uses vm.expectRevert() to indicate that the call reverts.

Run the test above by running forge test --mt test_lockedEther

Impact

Any ether sent to the TitlesGraph contract through its payable functions will be permanently locked and cannot be retrieved, leading to potential financial losses for users who accidentally or intentionally send ether to the contract.

Code Snippet

The test proves the exploit of funds being locked in the TitlesGraph.sol contract, specifically, the test code is exploiting the grantRoles function on line 158:

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/graph/TitlesGraph.sol?plain=1#L158

However, there is another function that can receive ether in the TitlesGraph.sol contract, such as the revokeRoles function on line 171:

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/graph/TitlesGraph.sol?plain=1#L171

Tool used

Foundry Manual Review

Recommendation

To mitigate this vulnerability, consider adding a withdraw function in the TitlesGraph contract that allows the contract owner or authorized parties to withdraw any ether held by the contract. This function should have appropriate access controls to ensure that only authorized entities can initiate the withdrawal.