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

Precision loss in `InheritanceManager::buyOutEstateNFT` leading to incorrect fund distribution.

roccomania

Summary

The buyOutEstateNFT function contains a vulnerability due to performing division before multiplication when calculating finalAmount. This can lead to precision loss and incorrect fund distribution, especially when dealing with small values or uneven divisions..

Vulnerability Details

https://github.com/CodeHawks-Contests/2025-03-inheritable-smart-contract-wallet/blob/main/src/InheritanceManager.sol#L263C1-L277C6
The vulnerability arises in the following line of code:

function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length;
uint256 multiplier = beneficiaries.length - 1;
@> uint256 finalAmount = (value / divisor) * multiplier;
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);
}

Here, value is divided by divisor (the number of beneficiaries) before being multiplied by multiplier (which is beneficiaries.length - 1). Since Solidity performs integer division, any remainder from the division is discarded, leading to precision loss. This can result in incorrect calculations, especially when value is not perfectly divisible by divisor.

For example:

  • If value = 100 and divisor = 3, then value / divisor = 33 (remainder 1 is discarded).

  • Multiplying 33 by multiplier (e.g., 2) gives 66, which is less than the expected 66.666....

This issue affects the fairness of fund distribution among beneficiaries and could lead to financial discrepancies.

Impact

  1. Precision Loss: The division before multiplication discards remainders, leading to incorrect calculations.

  2. Financial Discrepancies: Beneficiaries may receive less than their fair share of funds.

Tools Used

  1. Manual Code Review: Identified the division before multiplication issue.

Recommendations

To mitigate this issue, the calculation should be reordered to perform multiplication before division. This ensures that precision is preserved as much as possible. Here's the corrected code:

function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length;
uint256 multiplier = beneficiaries.length - 1;
- uint256 finalAmount = (value / divisor) * multiplier;
+ uint256 finalAmount = (value * multiplier) / divisor;
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);
}
```
​
This change ensures that the multiplication is performed first, minimizing precision loss.
Updates

Lead Judging Commences

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

truncation of integers

Support

FAQs

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