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

NFT Burning Before Guaranteed Transfers inside InheritanceManager::buyOutEstateNFT()

sala1

Summary

The nft.burnEstate(_nftID) call happens after the loop that distributes funds to the beneficiaries. If any of the safeTransfer calls within the loop fail (e.g., due to a beneficiary's contract reverting, or insufficient allowance), the NFT will still be burned, but not all beneficiaries will have received their intended share. This leads to an inconsistent state.

code in consideration:

/**
* @dev On-Chain payment of underlaying assets.
* CAN NOT use ETHER
* @param _nftID NFT ID to buy out
*/
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);
// <== above line could fail for whateve reason and line 21 below
// will still be hit!
}
}
nft.burnEstate(_nftID);
}

Impact

incorrect payment calculation. Its not evenly divided anymore.

Tools Used

Manual review

Recommendations

Move the nft.burnEstate(_nftID) call inside a conditional block that only executes if all the safeTransfer calls are successful. Even better to use an array to track transfer success, or reverting the entire transaction if any transfer fails.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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