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

Any deletion in InheritanceManager::beneficiaries breaks all logic based on array length, leading to wrongly burnt ETH and causing DOS in some functions

vanshikag

Summary

InheritanceManager::beneficiaries is an array of addresses. The function removeBeneficiary() resets the first instance of the address-to-remove to address(0). The array length remains the same. Some functions in InheritanceManager use the array's length as the number of addresses that the funds have to be split between and they do not account for deletions. Functions affected by this include withdrawInheritedFunds(), buyOutEstateNFT() and inherit(). The impact on each of these functions is demonstrated in the Proof Of Concept section. Using call() while iterating over the beneficiaries array leads to transfers to address(0), i.e., burnt ETH, for deleted array items. This call happens in the following line in withdrawInheritedFunds():

(bool success,) = beneficiary.call{value: amountPerBeneficiary}("");

Token transfers revert when asked to send tokens to address(0). This causes DOS of the buyOutEstateNFT() function as this does not allow ETH transfers and token transfers will always revert if there is a deletion in the array.

Impact

  • Portion of ETH which should be sent to beneficiaries of the contract is burnt instead.

  • DOS in token transfers executed by iterating over the array. (withdrawInheritedFunds(tokenAddr), and buyOutEstateNFT())

  • inherit() logic broken.

Tools Used

Manual Code Review, Foundry

Proof of Concept

Copy the following into the project test folder and run the tests.

​
//SPDX-License-Identifier: MIT
​
pragma solidity 0.8.26;
​
import {Test, console} from "forge-std/Test.sol";
import {InheritanceManager} from "../src/InheritanceManager.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/token/ERC20Mock.sol";
​
contract InheritanceManagerTest is Test {
InheritanceManager im;
ERC20Mock usdc;
ERC20Mock weth;
​
address owner = makeAddr("owner");
address alice = makeAddr("alice");
address bob = makeAddr("bob");
address charlie = makeAddr("charlie");
address dave = makeAddr("dave");
​
function setUp() public {
vm.prank(owner);
im = new InheritanceManager();
usdc = new ERC20Mock();
weth = new ERC20Mock();
}
​
function test_anyoneCanInheritIfArrayLengthIsOne() public {
vm.startPrank(owner);
im.addBeneficiery(alice); // beneficiaries.length = 1
assertEq(0, im._getBeneficiaryIndex(alice));
// beneficiaries.length remains 1 after this step. beneficiaries[0] = address(0)
im.removeBeneficiary(alice);
vm.stopPrank();
vm.warp(1 + 90 days);
vm.prank(alice);
im.inherit();
// anyone who calls inherit in this situation will be the new owner
assertEq(alice, im.getOwner());
}
​
function test_burntETHAndTokenTransferReverts() public {
vm.startPrank(owner);
im.addBeneficiery(alice);
im.addBeneficiery(bob);
im.addBeneficiery(charlie);
// beneficiaries.length = 3
im.removeBeneficiary(alice);
// total beneficiaries = 2. array length = 3.
im.createEstateNFT("our beach-house", 3000000, address(usdc));
vm.stopPrank();
vm.deal(address(im), 9e18);
vm.warp(1 + 90 days);
vm.startPrank(bob);
im.inherit();
im.withdrawInheritedFunds(address(0));
assertEq(3e18, bob.balance); // im.balance/3
assertEq(3e18, charlie.balance);
// 1/3rd of the balance burnt because beneficiaries[0] == address(0)
vm.expectRevert();
im.buyOutEstateNFT(1); // safeTransfer does not send tokens to address(0) and reverts transaction.
​
}
​
function test_repeatBeneficiary() public {
vm.startPrank(owner);
im.addBeneficiery(alice);
im.addBeneficiery(bob);
im.addBeneficiery(alice);
vm.stopPrank();
vm.deal(address(im), 9e18);
vm.warp(1 + 90 days);
vm.startPrank(alice);
im.inherit();
im.withdrawInheritedFunds(address(0));
assertEq(6e18, alice.balance);
assertEq(3e18, bob.balance);
}
}
​

Expected output:

[⠢] Compiling...
[⠆] Compiling 1 files with Solc 0.8.26
[⠰] Solc 0.8.26 finished in 3.08s
Compiler run successful!
​
Ran 4 tests for test/ProofOfConcept.t.sol:InheritanceManagerTest
[PASS] test_anyoneCanInheritIfArrayLengthIsOne() (gas: 72453)
[PASS] test_burntETHAndTokenTransferReverts() (gas: 401656)
[PASS] test_inheritanceDoesNotRemoveOwnerPrivileges() (gas: 195700)
[PASS] test_repeatBeneficiary() (gas: 235040)
Suite result: ok. 4 passed; 0 failed; 0 skipped; finished in 23.62ms (10.97ms CPU time)
​
Ran 1 test suite in 432.11ms (23.62ms CPU time): 4 tests passed, 0 failed, 0 skipped (4 total tests)

Recommendations

  1. Before each transfer, you can check for zero addresses to prevent call() sending ether to address(0) or token transfers reverting.

  2. You can use a mapping (address => bool) instead of an array to store the beneficiary addresses, and a counter to keep track of total addresses added/removed. This removes the problem of holes in an array from item deletion, and also improves calculations based on number of addresses. Additionally this prevents duplicate entries in beneficiaries.

  3. Instead of iterating over the addresses to send them the assets, you can implement a withdraw function that can be called by the beneficiaries. This function would check msg.sender against the mapping implemented and transfer the allocated funds to the calling address.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 3 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Incorrect removal from beneficiary list causes funds to be send to 0 address

0xtimefliez Lead Judge 3 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Incorrect removal from beneficiary list causes funds to be send to 0 address

Support

FAQs

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