Summary
The buyOutEstateNFT function is intended to facilitate a buyout of an NFT representing a real-world asset (RWA), but it does not transfer ownership of the NFT to the caller nor consistently burn it as expected. Instead, it burns the NFT only if the caller is not among the beneficiaries iterated before the return statement, effectively paying other beneficiaries without guaranteeing a change in ownership or removal of the NFT, rendering the payment potentially meaningless.
Vulnerability Details
The vulnerable code is in the buyOutEstateNFT function:
solidity
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);
}
POC
pragma solidity 0.8.26;
import {Test, console} from "forge-std/Test.sol";
import {InheritanceManager} from "../src/InheritanceManager.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/token/ERC20Mock.sol";
contract InheritanceManagerTest is Test {
InheritanceManager im;
ERC20Mock usdc;
ERC20Mock weth;
address owner = makeAddr("owner");
address user1 = makeAddr("user1");
function setUp() public {
vm.prank(owner);
im = new InheritanceManager();
usdc = new ERC20Mock();
weth = new ERC20Mock();
}
function test_buyOutEstateNFTDoesntChangeNftOwnershipOrBurnIt() public {
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
vm.warp(1);
vm.startPrank(owner);
im.addBeneficiery(user1);
im.addBeneficiery(user2);
im.addBeneficiery(user3);
im.createEstateNFT("our beach-house", 3e6, address(usdc));
vm.stopPrank();
usdc.mint(user3, 4e6);
usdc.mint(user2, 4e6);
vm.warp(1 + 90 days);
vm.startPrank(user3);
usdc.approve(address(im), 4e6);
im.inherit();
im.buyOutEstateNFT(1);
vm.stopPrank();
uint256 user3Bal1 = usdc.balanceOf(address(user3));
vm.startPrank(user2);
usdc.approve(address(im), 4e6);
im.buyOutEstateNFT(1);
vm.stopPrank();
uint256 user3Bal2 = usdc.balanceOf(address(user3));
assertEq(user3Bal1 + 3e6, user3Bal2, "user3 doesn't receive the nft money nor the nft was burnt");
}
}
No Ownership Transfer:
The function does not call any mechanism (e.g., nft.transferFrom or nft.safeTransferFrom) to transfer the NFT’s ownership to the caller, leaving it unclear who owns the NFT post-buyout.
The intent of a "buyout" typically implies the caller gains control, but this is not implemented.
-
Conditional Burn:
The nft.burnEstate(_nftID) call is only reached if the caller (msg.sender) is not found in the beneficiaries array before the loop completes.
If msg.sender matches a beneficiary, the return statement exits the function early, skipping the burn. This means the NFT remains intact, and the caller pays without receiving or destroying the asset.
-
Purpose Misalignment:
The function transfers funds to other beneficiaries, but without transferring or reliably burning the NFT, the payment appears to be for "nothing," contradicting the buyout’s purpose.
Impact
Wasted Funds: The caller pays other beneficiaries without gaining ownership or ensuring the NFT is removed, effectively losing funds for no benefit.
Ambiguous Ownership: The NFT remains in its original state (owned by the contract or original minter), undermining the buyout’s intent and potentially allowing multiple buyouts of the same asset.
User Confusion: Beneficiaries expect a buyout to transfer or retire the asset, but this inconsistency
Tools Used
Manual Review and Foundry
Recommendations
Ensure the NFT is transferred to the caller or consistently burned:
Option 1: Transfer Ownership
solidity
function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length - 1;
uint256 totalAmount = value;
IERC20(assetToPay).safeTransferFrom(msg.sender, address(this), totalAmount);
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender != beneficiaries[i]) {
IERC20(assetToPay).safeTransfer(beneficiaries[i], value / divisor);
}
}
nft.safeTransferFrom(address(this), msg.sender, _nftID);
}
Option 2: Consistent Burn
solidity
function buyOutEstateNFT(uint256 _nftID) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length - 1;
uint256 totalAmount = value;
IERC20(assetToPay).safeTransferFrom(msg.sender, address(this), totalAmount);
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender != beneficiaries[i]) {
IERC20(assetToPay).safeTransfer(beneficiaries[i], value / divisor);
}
}
nft.burnEstate(_nftID);
}
