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

The system will transfer funds to address(0) when executing `InheritanceManager::withdrawInheritedFunds` in case of `InheritanceManager::removeBeneficiary`.

luongtrongdai

Description: After the owner executes InheritanceManager::removeBeneficiary, the beneficiary address is updated to address(0). Therefore, when someone calls InheritanceManager::withdrawInheritedFunds, a portion of the funds will be sent to address(0).

Impact: The beneficiaries will not receive their full amount. A portion of the funds will be lost.

Proof of Concept: Add a new test to call withdrawInheritedFunds in case someone is removed.

function test_lostFundToAddressZeroBeneficiaries() public {
address user2 = makeAddr("user2");
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.removeBeneficiary(user1);
vm.stopPrank();
vm.warp(1);
vm.deal(address(im), 10e10);
vm.warp(1 + 90 days);
vm.startPrank(user1);
im.inherit();
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
uint256 userBalance = user1.balance + user2.balance;
assertEq(0, address(im).balance);
assertEq(10e10, userBalance);
}

After all transactions, the total funds received by users is 5e10 instead of 10e10. A portion of the funds is lost.

Recommended Mitigation: We should update the code as follows:

  1. Count the number of beneficiaries, excluding the removed beneficiary.

  2. Check address(0) before transfer fund.

+ function _countDevisor() internal view returns (uint256 divisor) {
+ uint256 nBeneficiaries = beneficiaries.length;
+ for (uint256 i = 0; i < nBeneficiaries; i++) {
+ if (beneficiaries[i] != address(0)) {
+ divisor++;
+ }
+ }
+ }
​
function withdrawInheritedFunds(address _asset) external {
if (!isInherited) {
revert NotYetInherited();
}
- uint256 divisor = beneficiaries.length;
+ uint256 divisor = _countDevisor();
+ uint256 nBeneficiaries = beneficiaries.length;
if (_asset == address(0)) {
uint256 ethAmountAvailable = address(this).balance;
uint256 amountPerBeneficiary = ethAmountAvailable / divisor;
- for (uint256 i = 0; i < divisor; i++) {
+ for (uint256 i = 0; i < nBeneficiaries; i++) {
address payable beneficiary = payable(beneficiaries[i]);
+ if (beneficiary != address(0)) {
(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++) {
+ for (uint256 i = 0; i < nBeneficiaries; i++) {
+ if (beneficiary != address(0)) {
IERC20(_asset).safeTransfer(beneficiaries[i], amountPerBeneficiary);
+ }
}
}
}
Updates

Lead Judging Commences

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

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

functions do not reset the deadline

Support

FAQs

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