Inheritable Smart Contract Wallet
First Flight #35
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

Bad Beneficiary Address permanently locks Funds in `InheritanceManager`after 90 day timelock

blackgrease

Summary:

A beneficiary address that does not receive funds can cause neither of the beneficiaries to be able to get their inheritance and permanently locks funds/assets in the contract.

Description:

In the InheritanceManager.sol contract, if a beneficiaries address is a address/smart contract that for one-reason or another rejects/reverts the funds, the withdrawal of the inheritance funds/assets fails for all of the beneficiaries. This results in the funds/assets being stuck indefinitely in the smart contract (assuming the owner does not make any interaction after the 90 day period).

This issue is caused by the handling of the transfers being executed as a batch rather than being handled independently. The logic which sends the inheritance funds is:

​
uint256 divisor = beneficiaries.length;
if (_asset == address(0)) {
uint256 ethAmountAvailable = address(this).balance; //audit-question: is this math correct? do they receive this amount-> what if ether is like 10.56 ether
uint256 amountPerBeneficiary = ethAmountAvailable / divisor;
for (uint256 i = 0; i < divisor; i++) {
address payable beneficiary = payable(beneficiaries[i]);
(bool success,) = beneficiary.call{value: amountPerBeneficiary}(""); //@audit-high: if one of the transfer fails, can i claim another ones inheritance?...no...funds are locked idenfinetly
require(success, "something went wrong");
}
}

This is unfair for the other beneficiaries who are able to accept the inheritance funds. Furthermore this breaks the contracts functionality as none of the beneficiaries are able to withdraw their share of the funds/assets.

Impact:

The impact of this vulnerability is High as funds are locked within the contract indefinitely. Furthermore, the contracts functionality is broken as none of the beneficiaries are able to obtain any of their shares (which defeats the purpose of the contract). The Likelihood is medium as there are variety of reasons a transaction may fail.

Tools used:

  • Manual Review

  • Foundry for testing

PoC:

To prove the validty of this issue, I have created a test function that can be run by: forge test --mt testWithdrawWithBadBenefector -vvv

The function:

  1. Adds inheritance funds and assigns 2 good beneficiaries and 1 beneficiary that has a smart contract address (contains a revert statement)

  2. Shows the inheritance balance

  3. Fast forwards to beyond the 90 day time period making locked funds eligibe for unlocking.

  4. Unlocks the funds and starts the inheritance process

  5. Shows that none of the beneficiaries received a balance.

The test function

contract InheritanceManagerTest is Test {
InheritanceManager im;
ERC20Mock usdc;
ERC20Mock weth;
BadContract badcontract;
​
address owner = makeAddr("owner");
address user1 = makeAddr("user1");
address user2 = makeAddr("user2");
​
function setUp() public {
vm.prank(owner);
im = new InheritanceManager();
usdc = new ERC20Mock();
weth = new ERC20Mock();
badcontract = new BadContract();
}
​
function testWithdrawWithBadBenefector() public {
//Arrange: Adding assets to the contract and assigning beneficiaries
vm.startPrank(owner);
vm.deal(address(im), 100 ether);
console.log("Starting wallet balance: ", address(im).balance);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(address(badcontract));
vm.stopPrank();
​
bool StartinginheritState = im.getIsInherited();
uint256 startingWalletBalance = address(im).balance;
​
//Act: Set timer past 90 days and calling inherit as beneficiary user and dispersing funds
vm.warp(1 + 90 days);
​
vm.startPrank(user1);
im.inherit();
bool NewInheritState = im.getIsInherited();
vm.expectRevert();
im.withdrawInheritedFunds(address(0));
uint256 finalWalletBalance = address(im).balance;
​
//Assert: the contract balance remains the same as no funds are dispersed due to the one bad address
assert(StartinginheritState == false);
assert(NewInheritState == true);
assert(startingWalletBalance == finalWalletBalance);
​
console.log("Wallet balance after inheritied: ", finalWalletBalance);
console.log("Beneficiery 1 balance: ", address(user1).balance);
console.log("Beneficiery 2 balance: ", address(user2).balance);
console.log("Beneficiery 3 balance: ", address(badcontract).balance);
}
}

The "bad" address

​
contract BadContract{
receive() external payable{
revert();
}
}

Recommended Mitigation:

The recommend fix for this issue is to

  1. Store the balances for each of the beneficiaries separately and then handle the transfers independently. This is so that, even if one of the addresses has an issue, it does not affect the flow of other beneficiaries gaining their inheritance funds.

  2. There could be a check to make sure that smart contract addresses are not allowed; However, this may be restrictive therefore the 1st recommendation may be more preferred.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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