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

[H-5] Incorrect `InheritanceManager::_getBeneficiaryIndex()` Implementation Allows Accidental Deletion of Slot 0 in the array, Causing Fund Loss and Potential DoS

agmguindo

Description:

The _getBeneficiaryIndex() function returns 0 when the given address is not found in the beneficiaries array. However, it also correctly returns 0 when the first beneficiary is actually present. This leads to a dangerous edge case in InheritanceManager::removeBeneficiary():

If _getBeneficiaryIndex() returns 0 because the given address is not in the array, the function wrongly deletes the beneficiary at slot 0.

This can result in unintended loss of a valid beneficiary.

Funds Loss: If slot 0 is deleted, future fund transfers might be sent to address(0), effectively burning funds.

DoS Vulnerability: In case of ERC-20 transfers, sending to address(0) triggers an ERC20InvalidReceiver error, reverting the entire transaction. This means that no other beneficiaries will receive funds, causing a denial of service (DoS).

​
function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
delete beneficiaries[indexToRemove];
}
​
function _getBeneficiaryIndex(address _beneficiary) public view returns (uint256 _index) {
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (_beneficiary == beneficiaries[i]) {
_index = i;
break;
}
}
}
​

Impact:

High Severity

Likelihood: High – If a non-existent address is provided, the first beneficiary is always deleted.

Loss of Beneficiary: A valid beneficiary could be wrongfully removed.

Funds Loss: If slot 0 is deleted, future ETH or ERC-20 transfers may be sent to address(0), effectively burning funds.

DoS Attack: If ERC-20 transfers are attempted to address(0), the transaction will revert due to ERC20InvalidReceiver error, preventing all other beneficiaries from receiving funds.

Proof of Concept

PoC
​
function test_audit_Return_added_to_loop() public {
vm.deal(address(im), 10e18);
vm.startPrank(owner);
string memory _description = "pepe";
uint256 _value = 1000000;
address _asset = address(usdc);
im.createEstateNFT(_description, _value, _asset);
uint256 valueOfNft = im.getNftValue(1);
console.log(valueOfNft);
console.log(nft.ownerOf(1));
​
im.addBeneficiery(address(user1));
im.addBeneficiery(address(user2));
im.addBeneficiery(address(user3));
im.addBeneficiery(address(user4));
​
uint256 deadLine2 = im.getDeadline();
console.log(deadLine2);
vm.warp(deadLine2 + 90 days);
​
//deleting the beneficiary in slot 0 by error
uint256 IdBeforeRemove = im._getBeneficiaryIndex(address(user1));
console.log(
"This should be index 0 representing user1: ",
IdBeforeRemove
);
im.removeBeneficiary(address(1));
​
uint256 IdAfterRemove = im._getBeneficiaryIndex(address(user1));
console.log(
"This should be index 0 representing emptyUser: ",
IdAfterRemove
);
vm.stopPrank();
​
vm.startPrank(user4);
im.inherit();
// we will loose funds wehn sending to no one in slot 0 or in case of ERC20 we will revert all Tx because
// of ERC20InvalidReceiver error when sending to address(0)
im.withdrawInheritedFunds(address(0));
usdc.mint(address(user4), 10e6);
usdc.approve(address(im), type(uint256).max);
​
im.buyOutEstateNFT(1);
vm.stopPrank();
}

Logs: InheritanceManager::withdrawInheritedFunds(0x0000000000000000000000000000000000000000)
@> [0] 0x0000000000000000000000000000000000000000::fallback{value: 2500000000000000000}() //burning funds
[Stop]
[0] user2::fallback{value: 2500000000000000000}()
[Stop]
[0] user3::fallback{value: 2500000000000000000}()
[Stop]
[0] user4::fallback{value: 2500000000000000000}()
[Stop]

Logs: InheritanceManager::buyOutEstateNFT(1)
ERC20Mock::transfer(0x0000000000000000000000000000000000000000, 187500 [1.875e5])
[Revert] ERC20InvalidReceiver(0x0000000000000000000000000000000000000000)

Recommended Mitigation:

To fix this issue, _getBeneficiaryIndex() should explicitly return an invalid index (e.g., type(uint256).max) when the address is not found. removeBeneficiary() should then check if the returned index is valid before deleting:

function removeBeneficiary(address _beneficiary) external onlyOwner {
uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
+ require(indexToRemove < beneficiaries.length, "Beneficiary not found");
delete beneficiaries[indexToRemove];
}
​
function _getBeneficiaryIndex(address _beneficiary) public view returns (uint256 _index) {
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (_beneficiary == beneficiaries[i]) {
return i;
}
}
return type(uint256).max; // Return an invalid index when not found
}
Updates

Lead Judging Commences

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

Give us feedback!