DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Funds can be lost using unsafe `low-level` call

alymurtazamemon

Summary

The summary of the below detail is that the low-level calls bypass type checking, function existence check, and argument packing and due to the fact that the EVM considers a call to a non-existing contract to always succeed makes the below-mentioned code vulnerable.

Vulnerability Detail

function unstake(address to, uint256 amount) external onlyDiamond {
IRocketTokenRETH rocketETHToken = _getRethContract();
​
uint256 rethValue = rocketETHToken.getRethValue(amount);
​
uint256 originalBalance = address(this).balance;
​
rocketETHToken.burn(rethValue);
​
uint256 netBalance = address(this).balance - originalBalance;
​
if (netBalance == 0) revert NetBalanceZero();
​
(bool sent, ) = to.call{value: netBalance}("");
​
assert(sent);
}

BridgeReth.sol - Line 98 - 107

The unstake function of the BridgeReth contract after burning the rocketETHToken and validating the netBalance calls the to contract using the low-level call and checks only the sent state of it.

This is unsafe because the low-level calls return true for all non-existing contract addresses.

Note: This function also does not have any check for address(0) but a non-existing contract can be a non-zero address.

Solidity Documentation Warning about low-level calls

  • You should avoid using .call() whenever possible when executing another contract function as it bypasses type checking, function existence check, and argument packing.

  • Due to the fact that the EVM considers a call to a non-existing contract to always succeed, Solidity includes an extra check using the extcodesize opcode when performing external calls. This ensures that the contract that is about to be called either actually exists (it contains code) or an exception is raised. The low-level calls which operate on addresses rather than contract instances (i.e. .call(), .delegatecall(), .staticcall(), .send() and .transfer()) do not include this check, which makes them cheaper in terms of gas but also less safe.

Solidity Docs - Members of Address Types

Proof

// SPDX-License-Identifier: MIT
pragma solidity 0.8.0;
​
import "hardhat/console.sol";
​
contract ContractA {
function check(address _target) external payable returns(bool) {
console.log("called");
(bool success, bytes memory data) = _target.call{value: msg.value}("");
require(success, "CALL_FAILED");
​
console.log("succeed");
console.log(success);
return success;
}
}
​
contract ContractB {
function destroy() external {
console.log("destroyed");
selfdestruct(payable(address(0)));
}
}

I have used this code to demonstrate, you can paste it into Remix IDE and verify it as well.

  1. Deployed the ContractB which contains the selfdestruct function (copy the contract address for later).

1

  1. Then call the destroy function of ContractB which will destroy this contract. After that, this contract will not exist anymore but its code still will be available on the blockchain.

2

  1. Deploy the ContractA

3

  1. Call the check function with the destroyed contract address and any msg.value.

4

As you can see the call went successful but the contract does not exist.

Impact

While unstaking the BridgeReth contract can accidentally lose funds by executing a call to a non-existing targe.

Tool used

Manual Review

Recommendation

Use the Openzeppelin Address library to perform the external call.

Updates

Lead Judging Commences

0xnevi Lead Judge
about 2 years ago
0xnevi Lead Judge about 2 years ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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