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

Reentrancy risk in withdrawInheritedFunds, CEI pattern not followed

mihaihng

Summary

withdrawInheritedFunds presents a reentrancy risk because the balance of the beneficiary is not updated before the external call to receive the funds.

Vulnerability Details

The function withdrawInheritedFunds does not respect the CEI pattern, threrefore it makes itself susceptible to reentrancy attacks, by a potentialy malicious contract that could recursively call the function to drain the contract funds.

Impact

The inheritanceManager contract could be drained of the funds that would correspond to its beneficiaries, resulting in loss of funds and no inheritance value.

Tools Used

Manual review

Recommendations

  1. Update balance of beneficiaries before the external calls are made to follow CEI(Checks-Effects-Interactions) pattern

  2. Use of pull-based merhod for withdrawing inherited funds

  3. Use the nonReentrant() modifier to prevent recursive calls.

​
mapping(address => uint256) public pendingWithdrawals; // Keeping track of beneficiaries balances
​
function withdrawInheritedFunds(address _asset) external nonReentrant {
if (!isInherited) {
revert NotYetInherited();
}
​
uint256 divisor = beneficiaries.length;
​
if (_asset == address(0)) {
uint256 ethAmountAvailable = address(this).balance;
uint256 amountPerBeneficiary = ethAmountAvailable / divisor;
​
// Update state first (CEI pattern)
for (uint256 i = 0; i < divisor; i++) {
pendingWithdrawals[beneficiaries[i]] += amountPerBeneficiary;
}
} 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);
}
}
}
​
// Beneficiaries must withdraw manually (pull-based)
function claim() external nonReentrant {
uint256 amount = pendingWithdrawals[msg.sender];
require(amount > 0, "Nothing to withdraw");
​
// Update state before transfer (CEI)
pendingWithdrawals[msg.sender] = 0;
​
(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Withdraw failed");
}
}
​
​
​

This code with the added changes:

-> prevents reentrancy(use nonReentrant modifier

-> follows CEI(Updates balances before making external calls)

-> avoids direct ETH transfers in loops(uses pull-based withdrawal pattern

-> protects against gas limits(Since each withdrawal is separate, no single transaction can fail due to out-of-gas errors)

Side note: I am submitting this entry from mobile, as I am away from my laptop for a couple of weeks, so I wasn't able to test this code, thank you for understanding.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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