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

Missing duplicate beneficiaries checks in `InheritanceManager.sol` results in unequal inheritances breaking protocl invariant

blackgrease

Summary:

The InheritanceManager.sol is missing checks on duplicate beneficiary addresses thus allowing a beneficiary to be entered more than once; resulting in the duplicate beneficiary to recieve more funds than other beneficiaries breaking the protocols invariant "shared equally".

Description:

The InheritanceManager.sol has two instances where checks for duplicate beneficiates could be implemented but are not.

1st Instance: addBeneficiery

The InheritanceManagerprovides a way for the owner to add beneficiaries using the function addBeneficiery. The function currently directly adds the provided address into the array of beneficiaries without any checks for duplicate addresses.

​
function addBeneficiery(address _beneficiary) external onlyOwner {
beneficiaries.push(_beneficiary);
_setDeadline();
}

2nd Instance: withdrawInheritedFunds

Once the contract is in an inheritable state and the funds/assets are being distributed via the withdrawInheritedFunds function, there is no check to see if there are any duplicate beneficiaries. Instead, the inheriticance is distributed by the number of beneficiaries and sent to each address regardless of the address apperaing more than once. The relevant code snippet is InheritanceManager.sol#L241

​
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");
}

Both areas of code could have a check for duplicate beneficiaries. The 1st instance may be gas costly as the check would have to loop the 2nd instance would have been cheaper and still as effective.

Furthermore, there is no direct way for an owner to fetch the array of beneficiaries resulting in poor visibility and manageent and increasing the chance for errors. While thereotically the InheritanceManger::_getBeneficiaryIndex could be used, it is not a feasible way to view as - quoting the documention - its intended purpose is "as a helper function for removeBeneficiary".

Impact:

The impact of missing checks for duplicate beneficiaries is HIGH due to when the inheritance is distributed amongst the beneficiaries, the duplicate beneficiaries receives a higher share than the others which further breaks the protocols invariant of "entirely equally divided" which is refering to beneficiaries getting equal shares. There is a medium likelihood of owner accidentally adding a beneficiary more than once.

Tools Used

  • Manual Review

  • Foundry for testing

PoC:

I have provided a Solidity test function to prove that the duplicate beneficiary address gets more funds than other beneficiaries.

Run using: forge test --mt testDuplicateAddressAndInheritance -vvv

  • 3vs to see the console log messages displayed.

The code:

  1. As the owner, beneficiaries are added. The benef_1 address is entered twice

  2. The 90 day timer is completed

  3. A beneficiary changes the contract state to be inheritable.

  4. The funds are withdrawn

  5. The balance of the duplicate beneficiary is compared to the balances of other beneficiaries to show that it is higher.

​
function testDuplicateAddressAndInheritance() public{
//Arrange: Adding funds 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(benef_1);
im.addBeneficiery(benef_2);
im.addBeneficiery(benef_3);
im.addBeneficiery(benef_4);
im.addBeneficiery(benef_1); //duplicate beneficiary
vm.stopPrank();
​
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(benef_1);
im.inherit();
bool NewInheritState = im.getIsInherited();
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
​
uint256 balanceOfDuplicateBeneficiary = address(benef_1).balance;
​
//Proving the Duplicate Beneficiary has obtained greater inheritance
assert(balanceOfDuplicateBeneficiary > address(benef_2).balance);
assert(balanceOfDuplicateBeneficiary > address(benef_3).balance);
assert(balanceOfDuplicateBeneficiary > address(benef_3).balance);
​
​
//Console Logs
console.log("Final wallet balance: ", address(im).balance);
console.log("Final Balance of Duplicate Beneficiary 1: ", address(benef_1).balance);
console.log("Final Balance of Beneficiary 2: ", address(benef_2).balance);
console.log("Final Balance of Beneficiary 3: ", address(benef_3).balance);
console.log("Final Balance of Beneficiary 4: ", address(benef_4).balance);
}
​

Recommended Mitigation:

The recommend mitigation to fix this issue is to implement a check for duplicate beneficiary addresses.

  • The 1st instance - addBeneficiery - may be gas costly as the check would have to loop seach time the function was called.

  • The 2nd instance - withdrawInheritedFunds - would be a more gas efficient as it only runs once and it would still be effective but its logic may be more complex as it would involve removing the duplicate address.

An example of the code for a duplicate address check in the first instance:

function addBeneficiery(address _beneficiary) external onlyOwner {
+ for (uint256 i = 0; i < beneficiaries.length; i++) {
+ if (_beneficiary == beneficiaries[i]) {
+ revert("Duplicate Beneficiary address");
+ }
+ }
beneficiaries.push(_beneficiary);
_setDeadline();
}
​
​

Another recommendation is including a function for the owner to have better visibility into the array of beneficiaries in order to catch any beneficiaries entered more than once. However, the 1st mitigation is highly recommended.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 4 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.