diff --git a/contracts/Aavegotchi/facets/WearablesConfigFacet.sol b/contracts/Aavegotchi/facets/WearablesConfigFacet.sol index 7e79225e..fe17e3a2 100644 --- a/contracts/Aavegotchi/facets/WearablesConfigFacet.sol +++ b/contracts/Aavegotchi/facets/WearablesConfigFacet.sol @@ -84,6 +84,11 @@ contract WearablesConfigFacet is Modifiers { fee += WEARABLESCONFIG_OWNER_FEE; } + // Effects before interactions: store the config and increment the slot counter before any ETH transfers. + WearablesConfig memory wearablesConfig = WearablesConfig({name: _name, wearables: _wearablesToStore}); + s.gotchiWearableConfigs[_tokenId][owner].push(wearablesConfig); + s.ownerGotchiSlotsUsed[owner][_tokenId] += 1; + if (fee > 0) { require(msg.value == fee, "WearablesConfigFacet: Incorrect GHST value sent"); @@ -104,11 +109,6 @@ contract WearablesConfigFacet is Modifiers { } } - // create the new wearables config and add it to the gotchi for that owner - WearablesConfig memory wearablesConfig = WearablesConfig({name: _name, wearables: _wearablesToStore}); - s.gotchiWearableConfigs[_tokenId][owner].push(wearablesConfig); - s.ownerGotchiSlotsUsed[owner][_tokenId] += 1; - emit WearablesConfigCreated(owner, _tokenId, wearablesConfigId, _wearablesToStore, msg.value); } diff --git a/contracts/test/WearablesConfigReenterer.sol b/contracts/test/WearablesConfigReenterer.sol new file mode 100644 index 00000000..fff7e57d --- /dev/null +++ b/contracts/test/WearablesConfigReenterer.sol @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.1; + +interface IWearablesConfigFacet { + function createWearablesConfig( + uint256 _tokenId, + string calldata _name, + uint16[16] calldata _wearablesToStore + ) external payable returns (uint16 wearablesConfigId); +} + +/// @dev Test helper that reenters WearablesConfigFacet.createWearablesConfig when it receives ETH. +contract WearablesConfigReenterer { + address public immutable diamond; + uint256 public tokenId; + string public innerName; + bool public didReenter; + + constructor(address _diamond) { + diamond = _diamond; + } + + function configure(uint256 _tokenId, string calldata _innerName) external { + tokenId = _tokenId; + innerName = _innerName; + } + + receive() external payable { + if (didReenter) return; + didReenter = true; + + uint16[16] memory wearables; + IWearablesConfigFacet(diamond).createWearablesConfig(tokenId, innerName, wearables); + } +} + diff --git a/test/wearablesConfigReentrancy.test.ts b/test/wearablesConfigReentrancy.test.ts new file mode 100644 index 00000000..0a781aec --- /dev/null +++ b/test/wearablesConfigReentrancy.test.ts @@ -0,0 +1,109 @@ +import { ethers } from "hardhat"; +import { expect } from "chai"; + +import { deploy } from "../js/diamond-util/src"; +import { DeploymentConfig } from "../scripts/deployFullDiamond"; + +import { AavegotchiBridgeFacet, WearablesConfigFacet, WearablesConfigReenterer } from "../typechain"; + +describe("WearablesConfigFacet: reentrancy", function () { + it("stores the config before paying the owner fee (prevents slot/id desync on reentrancy)", async function () { + this.timeout(300000); + + const [deployer, caller] = await ethers.getSigners(); + + const chainId = (await ethers.provider.getNetwork()).chainId; + const deploymentConfig = { chainId } as DeploymentConfig; + + const initArgs = [ + [ + deployer.address, // dao + deployer.address, // daoTreasury + deployer.address, // pixelCraft + deployer.address, // rarityFarming + deployer.address, // ghstContract (unused here) + deployer.address, // vrfSystem (unused here) + deployer.address, // relayerPetter (unused here) + ], + ]; + + const { deployedDiamond } = await deploy({ + diamondName: "AavegotchiDiamond", + initDiamond: "contracts/Aavegotchi/InitDiamond.sol:InitDiamond", + facetNames: ["WearablesConfigFacet", "AavegotchiBridgeFacet"], + signer: deployer, + args: initArgs, + deploymentConfig, + }); + + const diamondAddress = deployedDiamond.address; + + const bridge = (await ethers.getContractAt( + "AavegotchiBridgeFacet", + diamondAddress, + deployer + )) as AavegotchiBridgeFacet; + + const wearablesConfig = (await ethers.getContractAt( + "WearablesConfigFacet", + diamondAddress, + caller + )) as WearablesConfigFacet; + + const reentererFactory = await ethers.getContractFactory( + "WearablesConfigReenterer", + deployer + ); + const reenterer = (await reentererFactory.deploy( + diamondAddress + )) as WearablesConfigReenterer; + await reenterer.deployed(); + + const tokenId = 1; + await reenterer.configure(tokenId, "inner"); + + // Bridge in a gotchi owned by the reenterer contract. + // Only the owner check matters here: WearablesConfigFacet requires status == AAVEGOTCHI. + const bridged = { + equippedWearables: Array(16).fill(0), + temporaryTraitBoosts: Array(6).fill(0), + numericTraits: Array(6).fill(0), + name: "reenterOwnerGotchi", + randomNumber: 0, + experience: 0, + minimumStake: 0, + usedSkillPoints: 0, + interactionCount: 0, + collateralType: ethers.constants.AddressZero, + claimTime: 0, + lastTemporaryBoost: 0, + hauntId: 1, + owner: reenterer.address, + status: 3, // LibAavegotchi.STATUS_AAVEGOTCHI + lastInteracted: 0, + locked: false, + escrow: ethers.constants.AddressZero, // ignored; setMetadata deploys a new escrow + respecCount: 0, + baseRandomNumber: 0, + }; + + await (await bridge.setMetadata([tokenId], [bridged])).wait(); + + const outerWearables = Array(16).fill(0); + + // Sender != owner, so an owner fee is paid to `owner` (reenterer), triggering reentrancy. + await ( + await wearablesConfig.createWearablesConfig(tokenId, "outer", outerWearables, { + value: ethers.utils.parseEther("0.1"), + }) + ).wait(); + + expect(await reenterer.didReenter()).to.equal(true); + expect(await wearablesConfig.getAavegotchiWearablesConfigCount(reenterer.address, tokenId)).to.equal(2); + + // The outer call should create id 0 (and persist it before paying the owner fee). + expect(await wearablesConfig.getWearablesConfigName(reenterer.address, tokenId, 0)).to.equal("outer"); + expect(await wearablesConfig.getWearablesConfigName(reenterer.address, tokenId, 1)).to.equal("inner"); + }); +}); +