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

Incorrect Calculation of Royalty and Code Continuation Issue, Beneficiary would not get their value worth

ssalaudeen10

Summary

The buyOutEstateNFT function is intended to handle the process of one beneficiary buying out an estate NFT from other beneficiaries. However, the current implementation contains critical issues:

  1. Incorrect Royalty Calculation: The calculation for the finalAmount is incorrect. It divides the total value by the number of beneficiaries and then multiplies by one less than the number of beneficiaries. This approach can lead to inaccuracies due to integer division and does not correctly represent the royalty distribution.

  2. Code Continuation Issue: The use of return within the loop causes the function to exit prematurely when the buying-out beneficiary is found, preventing the subsequent code from executing and potentially leading to unexpected behavior.

Vulnerability Details

The code below demonstrates how this issue can be

function test_inheritance_calculation_is_wrong()public{
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
uint256 value = 20 ether;
vm.startPrank(owner);
//Owner adds User1, User2, User3 as beneficiaries
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
//Owner creates a NFT indicating He/She owns a porche Car worth 20 USDC
im.createEstateNFT("A porche Car", value, address(usdc));
vm.stopPrank();
// Increase Block time stamp
vm.warp(block.timestamp + im.TIMELOCK() + 1 days);
​
vm.startPrank(user1);
// User one calls the inherit function for beneficiaries to take over the contract
im.inherit();
// Mint 20 USDC to theuser1
usdc.mint(user1, value);
// approve 20 USDC for Inheritance Manager contract
usdc.approve(address(im), value);
/**
The buyout function is expected to transfer 13.3 USDC from user 1 to the contract
USER2, USER3 should recieve 6.666666666666667 USDC which is there share from the buy out
*/
im.buyOutEstateNFT(1);
vm.stopPrank();
uint256 balanceOfUser2 = usdc.balanceOf(user2);
uint256 balanceOfUser3 = usdc.balanceOf(user3);
// asserting that both USER2 and USER3 receives no USDC due math error and loop error
vm.assertEq(balanceOfUser2, 0);
vm.assertEq(balanceOfUser3, 0);

Impact

Inaccurate calculation of royalties, resulting in incorrect payouts to beneficiaries and Premature termination of the function, leading to incomplete execution of the intended logic.

Tools Used

Foundry test

Recommendations

o address these issues, the following steps should be taken:

  1. Correct Royalty Calculation: Adjust the calculation to correctly account for the royalty distribution among beneficiaries.

  2. Ensure Complete Execution: Remove the return statement within the loop to ensure the function completes its intended logic.

contract InheritanceManager is Trustee{
​
+ function _filterBeneficiaries()internal view returns(address[] memory ){
+ uint256 listOfBeneficiaries = beneficiaries.length;
+ uint numberOfNoneAddressZero;
+ for(uint i =0; i < listOfBeneficiaries; i++){
+ if(beneficiaries[i] != address(0)){
+ numberOfNoneAddressZero ++;
+ }
+
+ }
+ address[] memory filteredAddressList = new address[](numberOfNoneAddressZero);
+ uint256 uniqueIndex;
+ for(uint i =0; i < listOfBeneficiaries; i++){
+ if(beneficiaries[i] != address(0)){
+ filteredAddressList[uniqueIndex] = beneficiaries[i];
+ uniqueIndex ++;
+ }
+
+ }
+ return filteredAddressList;
+ }
​
function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
+ address [] memory filteredBeneficiary = _filterBeneficiaries();
+ uint256 divisor = filteredBeneficiary.length;
+ uint256 multiplier = filteredBeneficiary.length - 1;
+ uint256 finalAmount = (value / divisor) * multiplier;
IERC20(assetToPay).safeTransferFrom(msg.sender, address(this), finalAmount);
for (uint256 i = 0; i < filteredBeneficiary.length; i++) {
+ if(filteredBeneficiary[i] != msg.sender){
+ IERC20(assetToPay).safeTransfer(filteredBeneficiary[i], finalAmount / multiplier);
​
}
}
nft.burnEstate(_nftID);
}
​
}
​
​
Updates

Lead Judging Commences

0xtimefliez Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

buyOutNFT has wrong denominator

buyOutNFT has return instead of continue

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!