Submission Details
Severity:
`InheritanceManager::onlyBeneficiaryWithIsInherited` modifier Consume more gas for performing inherited functionalities.
Description
After the wallet inherited by any of the beneficiary the contrat unlocks more functionalities for the wallet. There is used an array for stoing the address of beneficiary. If there is large number of beneficiary then it will be used that array for matching the beneficiary. but every time looping the array will cost more gas.
```javascript
modifier onlyBeneficiaryWithIsInherited() {
uint256 i = 0;
while (i < beneficiaries.length + 1) {
if (msg.sender == beneficiaries[i] && isInherited) {
break;
}
i++;
}
_;
}
```
Due to this DOS attack can be happened and the protocol will cost the user more gas.
Impact
user needs to pay unnecessary gas for the calling one function. results in DOS attack.
Proof of Concept
1. Add large number of beneficiary.
2. Call `buyOutEstateNFT` function with the las beneficiary (gasUsed: `7407335`)(Used the default modifier given by the developer)
3. Again call same function with the recomended modifier(gasUsed: `6778624`)
4. diffrence of gas used = `628711`
Can see the huge diffrence in gas used before and after
Add this into `InheritanceMAnagerTest.t.sol`
```javascript
function testGas() public {
vm.txGasPrice(1);
uint256 playerNum = 1000;
address[] memory beneficiaries = new address[](playerNum);
for(uint256 i=0; i< playerNum ; i++) {
beneficiaries[i] = address(uint160(uint256(keccak256(abi.encodePacked(i)))));
}
vm.startPrank(owner);
for(uint i =0;i<playerNum;i++) {
im.addBeneficiery(beneficiaries[i]);
}
im.createEstateNFT("abc0",20,address(usdc));
vm.stopPrank();
usdc.mint(beneficiaries[playerNum - 1], 50);
assertEq(usdc.balanceOf(beneficiaries[playerNum - 1]), 50);
vm.warp(im.getDeadline() + 90 days + 1);
vm.startPrank(beneficiaries[playerNum - 1]);
usdc.approve(address(im),50);
im.inherit();
uint256 startGas = gasleft();
im.buyOutEstateNFT(1);
uint256 endingGas = gasleft();
console.log("Gas used", (startGas - endingGas) * tx.gasprice);
vm.stopPrank();
}
```
and test agained both modifier.
Recommended Mitigation
For checking the beneficiary the contract should mot use lopping through array instead if that contract should map the beneficiary.
add the following in the contract for map the beneficiary weather the caller is part of beneficiary or not.
```javascript
mapping(address beneficiary => bool ok) isBeneficiary;
```
also modify the modifier like following.
```diff
modifier onlyBeneficiaryWithIsInherited() {
- uint256 i = 0;
- while (i < beneficiaries.length + 1) {
- if (msg.sender == beneficiaries[i] && isInherited) {
- break;
- }
- i++;
- }
+ require(isBeneficiary[msg.sender] && isInherited, "not beeficiary and not inherited.");
_;
}
```
Add the mapping to the required functionalities in contract where the beneficiary need to add or checked.
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.