all of the changes below are subject to contract InheritanceManager
Proof of Concept: Claiming Unbelongs Asset
Summary
This could be a vulnerability if owner intends to inherit more than one asset to multiple different inheritance.
However, if owner intends to inherit his/her asset(s) to only a fix group of inheritance, then this wouldnt be an issues
For example:
Owner has one/some asset(s), and owner wish to inherit all of his/her asset(s) to Alice && Bob && Kylie. Regardless of what type or how many asset to be inherited, three of them should receive all of the asset(s) equally.
If the owner has the similar intendation as the given scenario, then there wont be any issues regarding false claiming
Vulnerability Details
A scenario of false claiming could be seen as below:
-
User1 User2 User3 User4: being beneficiaries of the inheritance asset
-
there are two type of assets by owner to be inherited to:
-
since there's no variables or function that keep tracks of who should claim which asset, every NFT will be distributed to all beneficiaries, including some beneficieris who should not received this particular NFT, but other NFT
function test_canClaimNotBelongsTo() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
address user4 = makeAddr("user4");
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
im.addBeneficiery(user4);
im.createEstateNFT("our beach-house", 3e6, address(usdc));
im.createEstateNFT("our lovely-house", 5e6, address(usdc));
vm.stopPrank();
usdc.mint(user2, 4e6);
usdc.mint(user4, 6e6);
skip(90 days);
vm.startPrank(user2);
usdc.approve(address(im), 4e6);
im.inherit();
im.buyOutEstateNFT(1);
im.withdrawInheritedFunds(address(usdc));
console.log(ERC20Mock(address(usdc)).balanceOf(user1));
console.log(ERC20Mock(address(usdc)).balanceOf(user2));
console.log(ERC20Mock(address(usdc)).balanceOf(user3));
console.log(ERC20Mock(address(usdc)).balanceOf(user4));
vm.stopPrank();
vm.startPrank(user4);
usdc.approve(address(im), 6e6);
im.inherit();
im.buyOutEstateNFT(2);
console.log(ERC20Mock(address(usdc)).balanceOf(user1));
console.log(ERC20Mock(address(usdc)).balanceOf(user2));
console.log(ERC20Mock(address(usdc)).balanceOf(user3));
console.log(ERC20Mock(address(usdc)).balanceOf(user4));
vm.stopPrank();
}
Impact
Unfair claim
Tools Used
Manual Review/Foundry
Proof of Concept: High Gas Consumption
Summary
modifier InheritanceManager::onlyBeneficiaryWithIsInherited
function InheritanceManager::_getBeneficiaryIndex
function InheritanceManager::withdrawInheritedFunds
they loop through a dynamic array InheritanceManager::beneficiaries.
Vulnerability Details
The more beneficiaries the owner adds using the function InheritanceManager::addBeneficiery, the higher gas cost on certain function that required a loop on InheritanceManager::beneficiaries.
having 3 beneficieries cost 128717
function test_lessPeople() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
vm.stopPrank();
vm.deal(address(im), 9e18);
skip(91 days);
vm.startPrank(user1);
uint gasStart = gasleft();
im.inherit();
im.withdrawInheritedFunds(address(0));
uint gasEnd = gasleft();
vm.stopPrank();
console.log("having 3 beneficieries cost", gasStart - gasEnd);
}
having 5 beneficieries cost 198261
function test_morePeople() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
address user4 = makeAddr("user4");
address user5 = makeAddr("user5");
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
im.addBeneficiery(user4);
im.addBeneficiery(user5);
vm.stopPrank();
vm.deal(address(im), 9e18);
skip(91 days);
vm.startPrank(user1);
uint gasStart = gasleft();
im.inherit();
im.withdrawInheritedFunds(address(0));
uint gasEnd = gasleft();
vm.stopPrank();
console.log("having 5 beneficieries cost", gasStart - gasEnd);
}
Impact
High gas consumption on looping, may potentially causing Denial of Service (DoS)
Tools Used
Manual Review/Foundry
Recommendation
Use mapping, instead of dynamic array
State Variables
mapping isInherited keep track of each NFT whether or not has/ready to be inherited.
mapping beneficiaries stores all the NFT the beneficiary will inherit , including keeping tracks of whether such beneficiary has receive the payment
-
paid == 0: no such NFT
-
paid == 1: has such NFT, could be either waiting for inheritance, or inherited, but not payout
-
paid == 2: has paid to such beneficiary
Enum is recommended to keep track of paid status, but not gas efficient
mapping totalInheritance keep tracks of the total beneficiaries for each NFT to be inherited to.
"NFT" doesn't necessary an NFT, but any assets, including USDC, DAI, ETH .... the purpose of having such mappings is to keep tracks on who should inherit whch assets, including whether they've received or not
- address[] beneficiaries;
- bool isInherited = false;
+ mapping(uint nftId => bool Inherited) public isInherited;
+ mapping(address beneficiary => mapping(uint NFTID => uint8 paid)) public beneficiaries;
+ mapping(uint nftId => uint16 Inheritance) public totalInheritance;
onlyBeneficiaryWithIsInherited
+ error notAccessed();
.
.
.
/**
- * @dev this while loop will revert on array out of bounds if not
- * called by a beneficiary.
+ * @dev this check ensures that only caller whom specific NFT can be inherited to
+ * @param _nftId using this specific NFT to checks
*/
- modifier onlyBeneficiaryWithIsInherited() {
+ modifier onlyBeneficiaryWithIsInherited(uint _nftId) {
- uint256 i = 0;
- while (i < beneficiaries.length + 1) {
- if (msg.sender == beneficiaries[i] && isInherited) {
- break;
- }
- i++;
- }
+ if ( // this NFT must ready to be inherited && msg.sender is one of the beneficiary of this NFT, and has not paid out yet
+ !isInherited[_nftId] ||
+ beneficiaries[msg.sender][_nftId] != 1
+ ) revert notAccessed(); // or any other error you like
_;
}
addBeneficiery
/**
* @dev adds a beneficiary for possible inheritance of funds.
* @param _beneficiary beneficiary address
+ * @param _nftId NFT id
*/
- function addBeneficiery(address _beneficiary) external onlyOwner {
+ function addBeneficiery(address _beneficiary, uint _nftId) external onlyOwner {
- beneficiaries.push(_beneficiary);
+ beneficiaries[_beneficiary][_nftId] = 1; // 1 indicates has this NFT to be inherited to
+ totalInheritance[_nftId] ++; // increment the total inheritance of such NFT
_setDeadline();
}
removeBeneficiary
for the sake of gas cost, we won't put an assertion to checks ensuring such NFT should have not inherited !isInherited[_nftId] && such user do have this NFT to be inherited beneficiaries[_beneficiary][_nftId] == 1
/**
* @dev removes entries from beneficiaries in case inheritance gets revoked or
* an address needs to be replaced (lost keys e.g.)
* @param _beneficiary address to be removed from the array beneficiaries
+ * @param _nftId NFT ID
*/
- function removeBeneficiary(address _beneficiary) external onlyOwner {
+ function removeBeneficiary(address _beneficiary, uint _nftId) external onlyOwner {
+ totalInheritance[_nftId] --;
+ beneficiaries[_beneficiary][_nftId] = 0;
- uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);
- delete beneficiaries[indexToRemove];
}
_getBeneficiaryIndex
since there's no array, there's no need to have a function to look for an index for a particular beneficiary.
getIsInherited
- function getIsInherited() public view returns (bool) {
+ function getIsInherited(uint _nftId) public view returns (bool) {
+ return isInherited[_nftId];
- return isInherited;
}
inherit
/**
* @dev manages the inheritance of this wallet either
* 1. the owner lost his keys and wants to reclaim this contract from beneficiaries slot0
* 2. the owner was inactive more than 90 days and beneficiaries will claim remaining funds.
*/
- function inherit() external {
+ function inherit(uint _nftId) external {
if (block.timestamp < getDeadline()) {
revert InactivityPeriodNotLongEnough();
}
- if (beneficiaries.length == 1) {
+ uint TotalInheritance = totalInheritance[_nftId];
+ if (TotalInheritance == 1) {
+ owner = msg.sender;
_setDeadline();
- } else if (beneficiaries.length > 1) {
+ } else if (TotalInheritance > 1) {
- isInherited = true;
+ isInherited[_nftId] = true;
} else {
revert InvalidBeneficiaries();
}
}
addFund (new)
this is a brand new function that when you (owner) want to inherit your cryptos like ETH or any tokens like USDC
this function helps keeping track of the total value of crypto so that it will be distributed among all the right amount to all the correct beneficiaries
param _asset input address(0) to inherit ETH/GWEI/WEI ...
param sendAmount input 0 to send ETH/GWEI/WEI
nftId doesnt only apply to NFT, but also any asset to be inherited
+ function addFund(address _asset, uint sendAmount) external payable onlyOwner nonReentrant {
+ if (_asset == address(0)) {
+ require(msg.value > 0, "must send something to be inherited"); // checks
+ uint256 nftID = nft._incrementCounter(); // get ID
+ nftValue[nftID] = msg.value; // update value
+ } else {
+ require(sendAmount > 0, "must send something to be inherited"); // checks
+ IERC20(assetToPay).safeTransfer(address(this), sendAmount); // send
+ uint256 nftID = nft._incrementCounter(); // get ID
+ nftValue[nftID] = sendAmount; // update value
+ }
+ }
withdrawInheritedFunds
a loop that automatically withdraw to all benefeciaries has removed
to claim/withdraw, they've to manually do it themselves
- error NotYetInherited();
.
.
.
/**
- * @dev called by the beneficiaries to disperse remaining assets within the contract in equal parts.
+ * @dev called by the inherited beneficiary to claim their asset
* @notice use address(0) to disperse ether
* @param _asset asset address to disperse
*/
- function withdrawInheritedFunds(address _asset) external {
+ function withdrawInheritedFunds(address _asset, uint _nftId) external onlyBeneficiaryWithIsInherited(_nftId) {
- if (!isInherited) {
- revert NotYetInherited();
- }
- uint256 divisor = beneficiaries.length;
+ uint256 amountPerBeneficiary = nftValue[_nftId] / totalInheritance[_nftId];
if (_asset == address(0)) {
- uint256 ethAmountAvailable = address(this).balance;
- uint256 amountPerBeneficiary = ethAmountAvailable / divisor;
+ beneficiaries[msg.sender][_nftId] = 3; // indicates such beneficiary has claimed
- for (uint256 i = 0; i < divisor; i++) {
- address payable beneficiary = payable(beneficiaries[i]);
- (bool success,) = beneficiary.call{value: amountPerBeneficiary}("");
- require(success, "something went wrong");
- }
+ (bool success, ) = msg.sender.call{value: amountPerBeneficiary}("");
+ require(success, "something went wrong");
} else {
- uint256 assetAmountAvailable = IERC20(_asset).balanceOf(address(this));
- uint256 amountPerBeneficiary = assetAmountAvailable / divisor;
- for (uint256 i = 0; i < divisor; i++) {
- IERC20(_asset).safeTransfer(beneficiaries[i], amountPerBeneficiary);
- }
+ beneficiaries[msg.sender][_nftId] = 3;
+ IERC20(_asset).safeTransfer(msg.sender, amountPerBeneficiary);
}
}
buyOutEstateNFT
a loop that automatically withdraw to all benefeciaries has removed
to claim/withdraw, they've to manually do it themselves
/**
* @dev On-Chain payment of underlaying assets.
* CAN NOT use ETHER
* @param _nftID NFT ID to buy out
*/
- function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited(_nftID) {
+ function buyEstateNFT(uint256 _nftID) external onlyOwner {
- uint256 value = nftValue[_nftID];
- uint256 divisor = beneficiaries.length;
- uint256 multiplier = beneficiaries.length - 1;
- uint256 finalAmount = (value / divisor) * multiplier;
+ (uint finalAmount, ) = _calculate(_nftID);
IERC20(assetToPay).safeTransferFrom(msg.sender, address(this), finalAmount);
- for (uint256 i = 0; i < beneficiaries.length; i++) {
- if (msg.sender == beneficiaries[i]) {
- return;
- } else {
- IERC20(assetToPay).safeTransfer(beneficiaries[i], finalAmount / divisor);
- }
- }
nft.burnEstate(_nftID);
}
+ function chaimNFT(uint256 _nftId) external onlyBeneficiaryWithIsInherited(_nftId) {
+ beneficiaries[msg.sender][_nftId] = 2;
+ (uint finalAmount, uint divisor) = _calculate(_nftId);
+ IERC20(assetToPay).safeTransfer(msg.sender, finalAmount / divisor);
+ }
+ function _calculate(uint256 _nftID) internal view returns(uint, uint) {
+ uint256 value = nftValue[_nftId];
+ uint256 divisor = totalInheritance[_nftId];
+ uint256 multiplier = divisor - 1;
+ uint finalAmount = (value / divisor) * multiplier;
+ return (finalAmount, divisor);
+ }
