Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Possible burning of ETH during account rescue

crisscs

Summary

When attempting to rescue an account, in the case that we are sending ETH, there are no checks for whether the 'rescued' address is address(0x0) in - Rescuable::rescue, which can result in accidental burning of ETH.

The contract is an integral part of the functionality of the project as any vulnerabilities in it, can affect the derived classes

  • CapitalPool.sol

  • DeliveryPlace.sol

  • PreMarkets.sol

  • SystemConfig.sol

  • TokenManager.sol

Commit Hash: 04fd8634701697184a3f3a5558b41c109866e5f8

Repository URL: https://github.com/Cyfrin/2024-08-tadle/tree/main

Vulnerability Details

In the Rescuable::rescue function, in the case where token == address(0x0), as per the natspec, the function will consider the token as ether and will attempt to transfer it to address to. However, there is no check whether address to is address(0x0), therefore the owner can accidentally burn ETH on line 15, in the snippet below.

/**
* @notice The caller must be the owner.
* @dev Rescues an account.
* @param to The address of the account to rescue.
* @param token The token to rescue. If 0, it is ether.
* @param amount The amount to rescue.
* @notice The caller must be the owner.
*/
function rescue(
address to,
address token,
uint256 amount
) external onlyOwner {
if (token == address(0x0)) {
payable(to).transfer(amount);
} else {
_safe_transfer(token, to, amount);
}
​
emit Rescue(to, token, amount);
}

Impact

ETH could be accidentally burned by the owner when rescuing an account.

Proof of Concept

  1. Add the following Rescuable.t.sol contract to the test directory

  2. Run forge test --match-contract Rescuable

  3. Observe the result

    Ran 2 tests for test/Rescuable.t.sol:RescuableTest
    [PASS] test_givenAddressZeroTokenAndAddressTo_expectedBurningOfETH() (gas: 46918)
    [PASS] test_initialOwnerIsDeployer() (gas: 7614)
    Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 335.04µs (92.33µs CPU time)
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;
​
import {Test} from "forge-std/Test.sol";
import {Rescuable} from "../src/utils/Rescuable.sol";
​
contract RescuableTest is Test {
Rescuable rescuable;
address zeroAddress = address(0);
​
function setUp() public {
rescuable = new Rescuable();
}
​
function test_initialOwnerIsDeployer() public {
assertEq(rescuable.owner(), address(this));
}
​
function test_givenAddressZeroTokenAndAddressTo_expectedBurningOfETH() public {
vm.deal(address(rescuable), 1 ether);
uint256 amountToTransferToZeroAddress = 0.5 ether;
uint256 balanceBeforeAttempt = address(rescuable).balance;
​
rescuable.rescue(zeroAddress, zeroAddress, amountToTransferToZeroAddress);
​
uint256 balanceAfterTransfer = address(rescuable).balance;
uint256 balanceOfZeroAddress = address(0).balance;
assertTrue(balanceBeforeAttempt > balanceAfterTransfer);
assertEq(balanceBeforeAttempt - amountToTransferToZeroAddress, balanceAfterTransfer);
assertEq(amountToTransferToZeroAddress, balanceOfZeroAddress);
}
}

Tools Used

Recommendations

  1. Add a check for address(0x0) in the Rescuable::rescue function.

Updates

Lead Judging Commences

0xnevi Lead Judge
about 1 year ago
0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.