Summary
As stated in the first point of Core Assumptions and Invariants:
EVERY transaction the owner does with this contract must reset the 90 days timer
every interaction that the owner does with the contract should reset the 90 days timer and this included removing a beneficiary as well as it is also an interaction with the contract but removing a beneficiary does not reset the timer and this is not intended functionality.
Vulnerability Details
As it can be confirmed from following test that when we add first user the deadline is set after 90 days and after 10 days we add another user and this time again the deadline is set 90 days after this time and again after 10 days we remove a user but this time the deadline is not pushed back but it remains the same when we added the user2 so this confirms that the remove function doesn't change the deadline but according to specifications all interactions should change the deadline.
function test_removeBeneficiaryDoesNotUpdateDeadline() public {
address user2 = makeAddr("user2");
vm.startPrank(owner);
im.addBeneficiery(user1);
uint256 deadline = im.getDeadline();
uint256 expectedDeadline = 90 days;
assertGe(
deadline,
expectedDeadline,
"Deadline should be greater than 90 days"
);
vm.stopPrank();
vm.warp(10 days);
vm.startPrank(owner);
im.addBeneficiery(user2);
deadline = im.getDeadline();
expectedDeadline = expectedDeadline + 10 days;
assertGe(
deadline,
expectedDeadline,
"Deadline should be greater than 100 days"
);
vm.stopPrank();
vm.warp(10 days);
vm.startPrank(owner);
im.removeBeneficiary(user2);
deadline = im.getDeadline();
expectedDeadline = expectedDeadline + 10 days;
assertGe(
deadline,
expectedDeadline,
"Deadline should be greater than 110 days"
);
vm.stopPrank();
}
[FAIL: Deadline should be greater than 110 days: 8640000 < 9504000] test_removeBeneficiaryDoesNotUpdateDeadline() (gas: 116144)
Traces:
[116144] InheritanceManagerTest::test_removeBeneficiaryDoesNotUpdateDeadline()
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]
├─ [0] VM::label(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802], "user2")
│ └─ ← [Return]
├─ [0] VM::startPrank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [69020] InheritanceManager::addBeneficiery(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Stop]
├─ [292] InheritanceManager::getDeadline() [staticcall]
│ └─ ← [Return] 7776001 [7.776e6]
├─ [0] VM::assertGe(7776001 [7.776e6], 7776000 [7.776e6], "Deadline should be greater than 90 days") [staticcall]
│ └─ ← [Return]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::warp(864000 [8.64e5])
│ └─ ← [Return]
├─ [0] VM::startPrank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [23120] InheritanceManager::addBeneficiery(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802])
│ └─ ← [Stop]
├─ [292] InheritanceManager::getDeadline() [staticcall]
│ └─ ← [Return] 8640000 [8.64e6]
├─ [0] VM::assertGe(8640000 [8.64e6], 8640000 [8.64e6], "Deadline should be greater than 100 days") [staticcall]
│ └─ ← [Return]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::warp(864000 [8.64e5])
│ └─ ← [Return]
├─ [0] VM::startPrank(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266])
│ └─ ← [Return]
├─ [1984] InheritanceManager::removeBeneficiary(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802])
│ └─ ← [Stop]
├─ [292] InheritanceManager::getDeadline() [staticcall]
│ └─ ← [Return] 8640000 [8.64e6]
├─ [0] VM::assertGe(8640000 [8.64e6], 9504000 [9.504e6], "Deadline should be greater than 110 days") [staticcall]
│ └─ ← [Revert] Deadline should be greater than 110 days: 8640000 < 9504000
└─ ← [Revert] Deadline should be greater than 110 days: 8640000 < 9504000
Impact
This behavior deviate from the basic assumptions about the working of the system.
Tools Used
foundry test
Recommendations
_setDeadline(); needs to be called after the removal of a user.
