Submission Details
Severity:
No check for restricting duplicate beneficiary causes wrong inheritance calculation
Summary
The function `addBeneficiery` does not check if the beneficiery is already in added. So if a beneficiery is added multiple times mistakenly, he will get multiple shares from the inheritane although technically he should get one share.
Vulnerability Details
/**
* @dev adds a beneficiary for possible inheritance of funds.
* @param _beneficiary beneficiary address
*/
function addBeneficiery(address _beneficiary) external onlyOwner {
beneficiaries.push(_beneficiary);
_setDeadline();
}
This addBeneficiery function pushes new beneficiery without checking it its already added to the list.
Impact
In this test, we have added three users/benefecieries but one used is added twice. The inheritence should be split between all three equally but since one user is added twice so the inheritence is divided into four parts and the user added twice get two of the four shares from the inheritence where as he should get one third of the inheritence amount.
function test_addDuplicateBeneficiarySuccess() public {
// vm.skip(true);
address user2 = makeAddr("user2");
console.log("Owner address: ", im.getOwner());
vm.startPrank(owner);
im.addBeneficiery(user1);
vm.stopPrank();
assertEq(0, im._getBeneficiaryIndex(user1));
vm.startPrank(owner);
im.addBeneficiery(user1);
vm.stopPrank();
vm.startPrank(owner);
im.addBeneficiery(user2);
vm.stopPrank();
assertEq(2, im._getBeneficiaryIndex(user2));
address user3 = makeAddr("user3");
vm.startPrank(owner);
im.addBeneficiery(user3);
vm.stopPrank();
vm.warp(1);
vm.deal(address(im), 12e18);
vm.warp(1 + 90 days);
vm.startPrank(user1);
im.inherit();
im.withdrawInheritedFunds(address(0));
vm.stopPrank();
console.log("User1 balance :", user1.balance);
console.log("User2 balance :", user2.balance);
console.log("User3 balance :", user2.balance);
assertEq(3e18, user1.balance);
assertEq(3e18, user2.balance);
assertEq(3e18, user3.balance);
}
Tools Used
foundry test
[FAIL: assertion failed: 3000000000000000000 != 6000000000000000000] test\_addDuplicateBeneficiarySuccess() (gas: 308177)
Logs:
Owner address: 0x7c8999dC9a822c1f0Df42023113EDB4FDd543266
User1 balance : 6000000000000000000
User2 balance : 3000000000000000000
User3 balance : 3000000000000000000
Traces:
\[308177] InheritanceManagerTest::test\_addDuplicateBeneficiarySuccess()
├─ \[0] VM::addr(<pk>) \[staticcall]
│ └─ ← \[Return] user2: \[0x537C8f3d3E18dF5517a58B3fB9D9143697996802]
├─ \[0] VM::label(user2: \[0x537C8f3d3E18dF5517a58B3fB9D9143697996802], "user2")
│ └─ ← \[Return]
├─ \[2419] InheritanceManager::getOwner() \[staticcall]
│ └─ ← \[Return] owner: \[0x7c8999dC9a822c1f0Df42023113EDB4FDd543266]
├─ \[0] console::log("Owner address: ", owner: \[0x7c8999dC9a822c1f0Df42023113EDB4FDd543266]) \[staticcall]
│ └─ ← \[Stop]
├─ \[0] VM::startPrank(owner: \[0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← \[Return]
├─ \[67020] InheritanceManager::addBeneficiery(user1: \[0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← \[Stop]
├─ \[0] VM::stopPrank()
│ └─ ← \[Return]
├─ \[933] InheritanceManager::\_getBeneficiaryIndex(user1: \[0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) \[staticcall]
│ └─ ← \[Return] 0
├─ \[0] VM::assertEq(0, 0) \[staticcall]
│ └─ ← \[Return]
├─ \[0] VM::startPrank(owner: \[0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← \[Return]
├─ \[23120] InheritanceManager::addBeneficiery(user1: \[0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← \[Stop]
├─ \[0] VM::stopPrank()
│ └─ ← \[Return]
├─ \[0] VM::startPrank(owner: \[0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← \[Return]
├─ \[23120] InheritanceManager::addBeneficiery(user2: \[0x537C8f3d3E18dF5517a58B3fB9D9143697996802])
│ └─ ← \[Stop]
├─ \[0] VM::stopPrank()
│ └─ ← \[Return]
├─ \[1895] InheritanceManager::\_getBeneficiaryIndex(user2: \[0x537C8f3d3E18dF5517a58B3fB9D9143697996802]) \[staticcall]
│ └─ ← \[Return] 2
├─ \[0] VM::assertEq(2, 2) \[staticcall]
│ └─ ← \[Return]
├─ \[0] VM::addr(<pk>) \[staticcall]
│ └─ ← \[Return] user3: \[0xc0A55e2205B289a967823662B841Bd67Aa362Aec]
├─ \[0] VM::label(user3: \[0xc0A55e2205B289a967823662B841Bd67Aa362Aec], "user3")
│ └─ ← \[Return]
├─ \[0] VM::startPrank(owner: \[0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← \[Return]
├─ \[23120] InheritanceManager::addBeneficiery(user3: \[0xc0A55e2205B289a967823662B841Bd67Aa362Aec])
│ └─ ← \[Stop]
├─ \[0] VM::stopPrank()
│ └─ ← \[Return]
├─ \[0] VM::warp(1)
│ └─ ← \[Return]
├─ \[0] VM::deal(InheritanceManager: \[0x88F59F8826af5e695B13cA934d6c7999875A9EeA], 12000000000000000000 \[1.2e19])
│ └─ ← \[Return]
├─ \[0] VM::warp(7776001 \[7.776e6])
│ └─ ← \[Return]
├─ \[0] VM::startPrank(user1: \[0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← \[Return]
├─ \[22686] InheritanceManager::inherit()
│ └─ ← \[Stop]
├─ \[112282] InheritanceManager::withdrawInheritedFunds(0x0000000000000000000000000000000000000000)
│ ├─ \[0] user1::fallback{value: 3000000000000000000}()
│ │ └─ ← \[Stop]
│ ├─ \[0] user1::fallback{value: 3000000000000000000}()
│ │ └─ ← \[Stop]
│ ├─ \[0] user2::fallback{value: 3000000000000000000}()
│ │ └─ ← \[Stop]
│ ├─ \[0] user3::fallback{value: 3000000000000000000}()
│ │ └─ ← \[Stop]
│ └─ ← \[Stop]
├─ \[0] VM::stopPrank()
│ └─ ← \[Return]
├─ \[0] console::log("User1 balance :", 6000000000000000000 \[6e18]) \[staticcall]
│ └─ ← \[Stop]
├─ \[0] console::log("User2 balance :", 3000000000000000000 \[3e18]) \[staticcall]
│ └─ ← \[Stop]
├─ \[0] console::log("User3 balance :", 3000000000000000000 \[3e18]) \[staticcall]
│ └─ ← \[Stop]
├─ \[0] VM::assertEq(3000000000000000000 \[3e18], 6000000000000000000 \[6e18]) \[staticcall]
│ └─ ← \[Revert] assertion failed: 3000000000000000000 != 6000000000000000000
└─ ← \[Revert] assertion failed: 3000000000000000000 != 6000000000000000000\
Recommendations
Since there is already a function to check a users' index `_getBeneficiaryIndex`, it should be used in the addBenefeciery function to check if a user already exists then it should not add it again. This way the inheritence calculations will be correct.
Rewards breakdown
nSLOC
211This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.