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

Missing beneficiary calling check on `inherit` and `withdrawInheritedFunds` allows anyone to initiate the inheritance process

blackgrease

Summary:

In the InheritanceManager.sol contract the inherit() and withdrawInheritedFunds are only supposed to be called by beneficiaries but currently anyone can call them, even when they are not beneficiaries and start the inheritance process.

Description:

The InheritanceManager.sol has the beneficiary function inherit, which is used to change the state of the contract to inheritable which opens up access to any funds and assets. The beneficiary function withdrawInheritedFunds calculates how much each beneficiary will get and sends the funds/assets. Currently, both beneficiary functions do not have any checks to make sure they can only be called by beneficiaries.

The error InvalidBeneficiaries and modifier onlyBeneficiaryWithIsInherited exist but are not implemented within the above functions logic.

The inherit() missing modifier to check only beneficiaries can call

​
function inherit() external {//Code...}
​

The withdrawInheritedFunds missing modifier onlyBeneficiaryWithIsInherited

​
function withdrawInheritedFunds(address _asset) external {//Code...}

As the @dev information provides, these functions are only supposed to be callable by only beneficiaries. While, no one can steal the inheritance funds/assets via this method resulting in the impact being low, this still breaks the protocols functionality as the code logic is not working as supposed to.

Impact:

The impact is low as anyone can change the contract state once the 90 days has passed to inheritable and initiate the inheritance transfers which is against the protocols functionality and no funds are at risk. Furthermore, there is no profit for the attacker

Tools Used

  • Manual Review

  • Foundry for Testing

PoC:

I have included a test function that can be run using: `forge test --mt testAnyoneCanCallInherit -vvv`.

The function:

  1. Deposits 10 ether as the owner

  2. Adds 2 beneficiaries

  3. Fast forwards past the 90 day deadline

  4. Calls inherit as a random user to change the contract state to inheritable

  5. Calls withdrawInheritedFunds as a random user to initiate the inheritance transfers

  6. Displays logs showing wallet balance before and after along with the beneficiaries balances

​
function testAnyoneCanCallInherit() public {
//Arrange: Adding assets to the contract and assigning beneficiaries
vm.startPrank(owner);
vm.deal(address(im), 10 ether);
console.log("Starting wallet balance: ", address(im).balance);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
vm.stopPrank();
​
bool StartinginheritState = im.getIsInherited(); //will return false
uint256 startingWalletBalance = address(im).balance;
​
//Act: Set timer past 90 days and calling inherit as random user. Then dispering the funds in a griefing attack
vm.warp(1 + 90 days);
​
vm.startPrank(randomPerson);
im.inherit();
bool NewInheritState = im.getIsInherited();
im.withdrawInheritedFunds(address(0));
uint256 finalWalletBalance = address(im).balance;
​
//Assert: Proving anyone who is not the beneficiery can change contracts state to .
assert(StartinginheritState == false);
assert(NewInheritState == true);
assert(startingWalletBalance > finalWalletBalance);
assert(finalWalletBalance == 0);
​
console.log("Wallet balance after 90 days: ", address(im).balance);
console.log("Beneficiery 1 balance: ", address(user1).balance);
console.log("Beneficiery 2 balance: ", address(user2).balance);
​
​
}
}

Recommended Mitigation:

The recommended mitigation for this is

  1. Implement a modifier that checks if the caller to inherit is part of the is part of the beneficiaries list before executing. An example of this is:

+ modifier onlyBeneficiary(){
+ for(uint i=0; i<beneficiaries.length +1; i++){
+ if (msg.sender == beneficiaries[i] && isInherited) {
+ revert InvalidBeneficiaries();
+ }
+ }
+ _;
+ }
//Implementing Modifier
+ function inherit() external onlyBeneficiary {//code}
​

The modifier onlyBeneficiaryWithIsInherited exists but is not suitable for the inherit function as the modifier requires the current state to be isInherited = true. If its used, the contracts state will never be updated thus locking all funds forever and breaking contract logic.

  1. The modifier onlyBeneficiaryWithIsInherited should be implemented on the withdrawInheritedFunds

​
+ function withdrawInheritedFunds(address _asset) external onlyBeneficiaryWithIsInherited {//code}
Updates

Lead Judging Commences

0xtimefliez Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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