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

Looping through `InheritanceManager:beneficiaries` array to check whether the caller is a beneficiary is a potential denial of service (DoS) attack, incrementing gas costs for future entrants

vincent71399

Description: The InheritanceManager contract uses a loop to check whether the caller is a beneficiary in the onlyBeneficiaryWithIsInherited modifier.
However, the longer the beneficiaries array, the more gas is consumed, which could lead to a potential denial of service (DoS) attack.
Additionally, removeBeneficiary does not reduce the length of the beneficiaries array, increasing the risk of a Denial-of-Service (DoS) attack.

modifier onlyBeneficiaryWithIsInherited() {
uint256 i = 0;
@> while (i < beneficiaries.length + 1) {
if (msg.sender == beneficiaries[i] && isInherited) {
break;
}
i++;
}
_;
}
...
function removeBeneficiary(address _beneficiary) external onlyOwner {
...
@> delete beneficiaries[indexToRemove];
...
}

Impact: The gas cost of the onlyBeneficiaryWithIsInherited modifier increases as the number of beneficiaries grows, impact function buyOutEstateNFT and appointTrustee

Proof of Concept: Add the following test and run

function test_DoS() public {
uint256 gasCost1 = gasCostMock(0);
uint256 gasCost2 = gasCostMock(100);
uint256 gasCost3 = gasCostMock(200);
​
assertLt(gasCost1, gasCost2);
assertLt(gasCost2, gasCost3);
​
console.log("gasCost1: %s", gasCost1);
console.log("gasCost2: %s", gasCost2);
console.log("gasCost3: %s", gasCost3);
}
​
function gasCostMock(uint256 loop) public returns (uint256 gasCost){
vm.startPrank(owner);
for(uint160 i; i < loop; i++){
// simulate by adding and removing beneficiaries to increase the beneficiaries array length
im.addBeneficiery(beneficiary1);
im.addBeneficiery(beneficiary2);
im.removeBeneficiary(beneficiary1);
im.removeBeneficiary(beneficiary2);
}
im.addBeneficiery(beneficiary1);
im.addBeneficiery(beneficiary2);
vm.stopPrank();
​
vm.warp(im.getDeadline());
im.inherit();
​
uint256 gasStart = gasleft();
vm.prank(beneficiary1);
im.appointTrustee(otherUser);
gasCost = gasStart - gasleft();
}

Recommended Mitigation:
Use mapping(address->bool) to replace the array to check if the address is a beneficiary.

+ mapping(address => bool) public isBeneficiary;
...
modifier onlyBeneficiaryWithIsInherited() {
require(isBeneficiary(msg.sender), "Not a beneficiary");
require(isInherited, "Not yet inherited");
_;
}
​
function addBeneficiery(address _beneficiary) external onlyOwner {
...
+ isBeneficiary(_beneficiary) = true;
_setDeadline();
}
​
function removeBeneficiary(address _beneficiary) external onlyOwner {
...
+ isBeneficiary(_beneficiary) = false;
_setDeadline();
}
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!