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

[M-5] Missing Reentrancy Protection in buyOutEstateNFT Function

d33p

Summary

The buyOutEstateNFT function in the InheritanceManager contract lacks reentrancy protection despite making external calls to potentially arbitrary token contracts. While standard ERC20 implementations typically cannot reenter, this could be exploited if a malicious or non-standard token is used as the payment asset.

Vulnerability Details

The function makes external calls via safeTransferFrom and safeTransfer without the nonReentrant modifier that is applied to other sensitive functions in the contract:

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);
}

This function could potentially be vulnerable because:

  1. It relies on external token contract behavior which varies by implementation

  2. While standard ERC20 tokens cannot typically reenter, non-standard or malicious tokens could implement callbacks within their transfer functions

  3. State changes (NFT burning) occur after external calls

  4. There's no check to prevent the same NFT from being bought out multiple times if a reentrancy were possible

Impact

If a malicious or non-standard token is used as assetToPay, it could potentially:

  1. Implement callbacks in transfer functions that reenter the contract

  2. Manipulate the distribution of funds to beneficiaries

  3. Possibly prevent the NFT from being properly burned

  4. Disrupt the inheritance process

The severity is medium because:

  • It requires a malicious or non-standard token implementation to exploit

  • Standard, trusted ERC20 tokens would not be vulnerable

  • It contradicts the security pattern applied to other functions in the contract

  • Defense-in-depth principles suggest adding protection regardless

Tools Used

Manual code review

Recommended Mitigation

Add the nonReentrant modifier to the function and follow the Checks-Effects-Interactions pattern by updating state before making external calls:

function buyOutEstateNFT(
uint256 _nftID
) external onlyBeneficiaryWithIsInherited nonReentrant {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length;
uint256 multiplier = beneficiaries.length - 1;
uint256 finalAmount = (value / divisor) * multiplier;
// Mark NFT as handled first (state update)
nftValue[_nftID] = 0; // Zero out the value or use another mechanism to mark as processed
// Then perform external calls
IERC20(assetToPay).safeTransferFrom(
msg.sender,
address(this),
finalAmount
);
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender == beneficiaries[i]) {
continue; // Use continue instead of return to ensure NFT is burned
} else {
IERC20(assetToPay).safeTransfer(
beneficiaries[i],
finalAmount / divisor
);
}
}
nft.burnEstate(_nftID);
}
Updates

Lead Judging Commences

0xtimefliez Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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