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

Incorrect amountPerBeneficiary, finalAmount and finalAmount per beneficiary calculation

mihaihng

Summary

amountPerBeneficiary, finalAmount and finalAmount per beneficiary are variables used for the calculation of each beneficiary share to withdraw.

amountPerBeneficiary is used in withdrawInheritedFunds()

finalAmount and finalAmount per beneficiary are used in buyOutEstateNFT()

These variables which are of type uint256, are calculated by division of the total amount to inherit by the number of beneficiaries. Because Solidity doesn't support floating point/noninteger numbers, in situations where the result is a floatin point/noninteger number will result in incorrect calculations and amounts per beneficiaries.

For example:

ethAmountAvailable = 4 ethereum and beneficiaries.length = 3, the result amountPerBeneficiary wil equal 1.33, which Solidity will round to 1, resulting in .33 ethereum loss per beneficiary, here totaling 1 ethereum lost.

If ethAmountAvailable = 1 ethereum and beneficiaries.length = 3, the result amountPerBeneficiary wil equal 0.33, which Solidity will round to 0, resulting in .33 ethereum loss per beneficiary, here totaling 1 ethereum lost.

Vulnerability Details

This incorrect calculation will lead to loss of funds, stuck in the inheritanceManager contract

Impact

The result of the calculations for amountPerBeneficiary, finalAmount and finalAmount per beneficiary will be incorrect, therefore, beneficiaries will receive less value than expected or in some cases 0 value.

Tools Used

Manual review

Recommendations

Use fixed-point arithmetics, by introducing and using a precision factor, in this case the ETH decimals value

uint256 public immutable i_ethPrecision = 1e18; // precision multiplier
​
function withdrawInheritedFunds(address _asset) external {
if (!isInherited) {
revert NotYetInherited();
}
uint256 divisor = beneficiaries.length;
if (_asset == address(0)) {
uint256 ethAmountAvailable = address(this).balance;
uint256 amountPerBeneficiary = (ethAmountAvailable * i_ethPrecision) / divisor; // use of precision multiplier to allow correct division operation
for (uint256 i = 0; i < divisor; i++) {
address payable beneficiary = payable(beneficiaries[i]);
(bool success,) = beneficiary.call{value: amountPerBeneficiary}("");
require(success, "something went wrong");
}
} else {
uint256 assetAmountAvailable = IERC20(_asset).balanceOf(address(this));
uint256 amountPerBeneficiary = (assetAmountAvailable * i_ethPrecision)/ divisor;// use of precision multiplier to allow correct division operation
for (uint256 i = 0; i < divisor; i++) {
IERC20(_asset).safeTransfer(beneficiaries[i], amountPerBeneficiary);
}
}
}
​
function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length;
uint256 multiplier = beneficiaries.length - 1;
uint256 finalAmount = ((value * i_ethPrecision) / divisor) * multiplier; // use of precision multiplier to allow correct division operation
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 * i_ethPrecision) / divisor); // use of precision multiplier to allow correct division operations
}
}
nft.burnEstate(_nftID);
}

These changes should make the division operations correct, avoiding fund loss and contract incosistent behavior.

Side note: I am writing these submission on mobile, being away from laptop at the moment, so I can not test the code. Thanks for understanding.

Updates

Lead Judging Commences

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

buyOutNFT has wrong denominator

Support

FAQs

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

Give us feedback!