diff --git a/packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol b/packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol index 241e616..05035a1 100644 --- a/packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol +++ b/packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol @@ -78,14 +78,35 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, ReentrancyGuardUpgradeable, OF /// @dev Outbound (send) account daily limit tracking mapping(address => AccountLimit) public accountsDailyLimit; + /** + * @dev Holds only what the replay needs: the message, the Origin to replay it under, whether the request + * is still outstanding, and the clock for the optimistic window. + * @dev toAddress and amount are deliberately absent - both are pure functions of the message, and + * ReceiveRequestFailed already carries them for off-chain consumers. _executor and _extraData are + * absent too: OFTCore._lzReceive ignores both, and _extraData is not covered by the verified payload + * hash, so it is unauthenticated data not worth a slot. + * @dev Origin is flattened rather than nested because its bytes32 sender cannot share a slot: a nested + * Origin would occupy three slots, where this packs failed/timestamp/srcEid/nonce into one (21 of 32 + * bytes) and leaves sender its own. Parking a plain message therefore writes 5 slots, not 7, which + * is what keeps it inside the executor's gas allowance. + * @dev UPGRADE NOTE: this layout is NOT compatible with the previous one. Only the mapping's value layout + * changed - every other state variable keeps its slot - so an upgrade is safe for limits, fees and + * counters, but pre-existing failedReceiveRequests entries are misread: `failed` still decodes + * correctly, while timestamp/srcEid/nonce/sender come from the old toAddress/timestamp bytes and + * `message` from the old srcEid slot. Such a record cannot be approved (the garbage timestamp fails + * the optimistic check, and the replay reverts crediting a zero recipient), so it is inert rather + * than dangerous. Only testnets have ever been deployed; a mainnet deployment carrying parked + * requests would need them drained before this upgrade. + */ struct FailedReceiveRequest { bool failed; - address toAddress; uint64 timestamp; - uint256 amount; uint32 srcEid; + uint64 nonce; + bytes32 sender; + bytes message; } - /// @dev A mapping for failed requests + /// @dev A mapping for failed requests, keyed by the LayerZero message guid mapping(bytes32 => FailedReceiveRequest) public failedReceiveRequests; uint64 public constant OPTIMISTIC_WINDOW = 3 days; @@ -117,6 +138,10 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, ReentrancyGuardUpgradeable, OF /// @dev Event emitted when fees are collected event FeeCollected(address indexed recipient, uint256 amount); + /// @dev Event emitted when a send destroys the sender's tokens at the MinterBurner. The MinterBurner's + /// own TokensBurned names itself as the debited account, so this is what ties the burn to the sender. + event SenderTokensBurned(address indexed from, uint256 amount); + /// @dev Event emitted when bridge limits are updated event BridgeLimitsSet( uint256 dailyLimit, @@ -129,7 +154,7 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, ReentrancyGuardUpgradeable, OF /// @dev Event emitted when bridge pause status changes event BridgePaused(bool isPaused); - /// @dev Event emitted when a failed receive request is made + /// @dev Event emitted when a limit-blocked receive is registered for recovery event ReceiveRequestFailed(bytes32 indexed guid, address toAddress, uint256 amount, uint32 srcEid); /// @dev Event emitted when a failed receive request is approved @@ -279,6 +304,9 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, ReentrancyGuardUpgradeable, OF /** * @notice Approves a failed receive request with optimistic window or by owner + * @dev Replays the parked message through the inherited OFT receive logic rather than only crediting it, + * so a compose payload is still dispatched. That is what lets composed and plain messages share one + * failure path: parking is no longer lossy. * @dev Approved amounts intentionally skip the daily limit counters */ function approveFailedRequest(bytes32 _guid) external nonReentrant { @@ -286,10 +314,54 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, ReentrancyGuardUpgradeable, OF FailedReceiveRequest memory request = failedReceiveRequests[_guid]; require(request.timestamp + OPTIMISTIC_WINDOW < block.timestamp || msg.sender == owner(), 'optimistic period not ended or not owner'); require(request.failed, 'request not failed'); - // G$ mint calls the recipient's ERC777 hook, so state must be final before _credit + // G$ mint calls the recipient's ERC777 hook, so state must be final before the replay failedReceiveRequests[_guid].failed = false; - _credit(request.toAddress, request.amount, request.srcEid); emit FailedReceiveRequestApproved(_guid); + this.replayFailedRequest(Origin(request.srcEid, request.sender, request.nonce), _guid, request.message); + } + + /** + * @notice Replays an approved request's message through the inherited OFT receive logic + * @dev Self-call only. OFTCore._lzReceive takes the message as calldata while the parked copy lives in + * storage, so the replay has to cross an ABI boundary to re-encode it. It deliberately bypasses this + * contract's _lzReceive override: the limits were already adjudicated by the approval, and the + * endpoint consumed the message when it was parked, so this is the only way it can ever execute. + */ + function replayFailedRequest(Origin calldata _origin, bytes32 _guid, bytes calldata _message) external { + require(msg.sender == address(this), 'only self'); + // _message[:0] is an empty calldata slice: OFTCore._lzReceive ignores _executor and _extraData, + // and a literal cannot be passed where calldata is expected + super._lzReceive(_origin, _guid, _message, address(0), _message[:0]); + } + + /** + * @notice Decodes a parked request into the values the struct does not store + * @dev failedReceiveRequests already returns the raw message - bytes members are included in an + * autogenerated struct getter, only mappings and non-byte arrays are omitted - but toAddress and + * amount are derived rather than stored, so this saves callers decoding the OFT payload themselves. + * @param _guid The guid of the parked request + * @return toAddress The recipient the replay will credit + * @return amountLD The amount in local decimals, before the receive fee + * @return isComposed Whether the replay will also dispatch a compose payload + */ + function failedRequestDetails( + bytes32 _guid + ) external view returns (address toAddress, uint256 amountLD, bool isComposed) { + bytes memory message = failedReceiveRequests[_guid].message; + require(message.length >= 40, 'no parked message'); + return this.decodeOftMessage(message); + } + + /** + * @notice Decodes an OFT payload with the same codec the receive path uses + * @dev Takes calldata because OFTMsgCodec slices calldata, so failedRequestDetails reaches it through a + * static self-call - the same ABI-boundary step replayFailedRequest needs, and the reason this is + * a separate function rather than inlined. + */ + function decodeOftMessage( + bytes calldata _message + ) external view returns (address toAddress, uint256 amountLD, bool isComposed) { + return (_message.sendTo().bytes32ToAddress(), _toLD(_message.amountSD()), _message.isComposed()); } /** @@ -303,7 +375,8 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, ReentrancyGuardUpgradeable, OF /** * @notice Bridge closed / whitelist check only (no limit checks). - * @dev Revert on this path does not store to failedReceiveRequests. + * @dev Revert on this path does not store to failedReceiveRequests, so the message stays retryable + * at the endpoint instead of becoming a parked request. */ function _checkBridgeClosedAndWhitelisted(address _from) internal view returns (bool ok, string memory reason) { if (isClosed) return (false, 'closed'); @@ -319,7 +392,8 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, ReentrancyGuardUpgradeable, OF /** * @notice Bridge limits check only (minAmount, accountDailyLimit, txLimit, dailyLimit). - * @dev Assumes daily limit resets have already been applied. Failure on receive is stored in failedReceiveRequests. + * @dev Assumes daily limit resets have already been applied. Failure on receive is stored in + * failedReceiveRequests, for composed and plain messages alike. */ function _checkBridgeLimits( uint256 _amount, @@ -439,7 +513,9 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, ReentrancyGuardUpgradeable, OF /** * @notice Overrides the default _lzReceive function to enforce limits on receiving side. - * @dev Closed/not whitelisted: revert. Limits exceeded: store in failedReceiveRequests (composed messages revert instead). + * @dev Closed/not whitelisted: revert. Limits exceeded: park in failedReceiveRequests, whatever the + * message type. The whole message is parked, so approveFailedRequest can replay it in full and a + * compose payload survives - which is why the message type no longer changes the failure handling. */ function _lzReceive( Origin calldata _origin, @@ -461,17 +537,16 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, ReentrancyGuardUpgradeable, OF (ok, reason) = _checkBridgeLimits(amountLD, bridgeReceiveDailyLimit, account); if (!ok) { - // parking would drop the compose payload; reverting keeps the message retryable at the endpoint - if (_message.isComposed()) revert BRIDGE_LIMITS(reason); failedReceiveRequests[_guid] = FailedReceiveRequest( true, - toAddress, uint64(block.timestamp), - amountLD, - _origin.srcEid + _origin.srcEid, + _origin.nonce, + _origin.sender, + _message ); emit ReceiveRequestFailed(_guid, toAddress, amountLD, _origin.srcEid); - } else{ + } else { bridgeReceiveDailyLimit.bridged24Hours += amountLD; account.bridged24Hours += amountLD; super._lzReceive(_origin, _guid, _message, _executor, _extraData); @@ -504,7 +579,10 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, ReentrancyGuardUpgradeable, OF } /** - * @notice Pulls tokens from the sender (adapter is the spender) and burns them via the MinterBurner + * @notice Moves the sender's tokens straight to the MinterBurner (adapter is the spender) and burns them + * @dev One transfer rather than pulling into the adapter and pushing on: the adapter never custodies + * user funds and needs no allowance to the MinterBurner. The burn itself debits the MinterBurner, + * so SenderTokensBurned is emitted here to record whose tokens funded it. * @param _from The address to debit the tokens from * @param _amountLD The amount of tokens to send in local decimals * @param _minAmountLD The minimum amount to send in local decimals @@ -519,11 +597,14 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, ReentrancyGuardUpgradeable, OF uint32 _dstEid ) internal virtual override returns (uint256 amountSentLD, uint256 amountReceivedLD) { (amountSentLD, amountReceivedLD) = _debitView(_amountLD, _minAmountLD, _dstEid); - uint256 balanceBefore = innerToken.balanceOf(address(this)); - innerToken.safeTransferFrom(_from, address(this), amountSentLD); - require(innerToken.balanceOf(address(this)) - balanceBefore == amountSentLD, 'received amount mismatch'); - innerToken.forceApprove(address(minterBurner), amountSentLD); - require(minterBurner.burn(address(this), amountSentLD), 'GoodDollarOFTAdapter: Burn failed'); + uint256 balanceBefore = innerToken.balanceOf(address(minterBurner)); + innerToken.safeTransferFrom(_from, address(minterBurner), amountSentLD); + require( + innerToken.balanceOf(address(minterBurner)) - balanceBefore == amountSentLD, + 'received amount mismatch' + ); + require(minterBurner.burn(address(minterBurner), amountSentLD), 'GoodDollarOFTAdapter: Burn failed'); + emit SenderTokensBurned(_from, amountSentLD); } function _authorizeUpgrade(address) internal virtual override onlyOwner {} diff --git a/packages/bridge-contracts/contracts/oft/GoodDollarOFTMinterBurner.sol b/packages/bridge-contracts/contracts/oft/GoodDollarOFTMinterBurner.sol index 646237c..f6105b5 100644 --- a/packages/bridge-contracts/contracts/oft/GoodDollarOFTMinterBurner.sol +++ b/packages/bridge-contracts/contracts/oft/GoodDollarOFTMinterBurner.sol @@ -61,17 +61,21 @@ contract GoodDollarOFTMinterBurner is DAOUpgradeableContract { } /** - * @dev Burn the caller's tokens; the caller must have approved this contract - * @param _from Must be msg.sender (kept for IMintableBurnable compatibility) + * @dev Burn tokens the calling operator has already transferred to this contract + * @param _from Must be this contract - the account actually debited (kept for IMintableBurnable compatibility) * @param _amount The amount of tokens to burn * @return success True if the burn was successful + * @dev Only ever destroys this contract's own balance: it does not call transferFrom, so an operator + * cannot reach a third party's allowance and the L-01 property holds structurally. The operator + * transfers the tokens here first; whoever they originated from is recorded by the operator, not + * here, so nothing in this event can misrepresent where the tokens came from. */ function burn(address _from, uint256 _amount) external onlyOperators returns (bool) { - require(_from == msg.sender, 'can only burn own tokens'); + require(_from == address(this), 'can only burn own tokens'); uint256 balanceBefore = token.balanceOf(address(this)); - require(token.transferFrom(_from, address(this), _amount), 'TRANSFER_FROM_FAILED'); - require(token.balanceOf(address(this)) - balanceBefore == _amount, 'received amount mismatch'); + require(balanceBefore >= _amount, 'tokens not received'); token.burn(_amount); + require(token.balanceOf(address(this)) == balanceBefore - _amount, 'burned amount mismatch'); emit TokensBurned(_from, _amount, msg.sender); return true; diff --git a/packages/bridge-contracts/contracts/test/LayerZeroEndpointMock.sol b/packages/bridge-contracts/contracts/test/LayerZeroEndpointMock.sol index 53e461a..91ece7c 100644 --- a/packages/bridge-contracts/contracts/test/LayerZeroEndpointMock.sol +++ b/packages/bridge-contracts/contracts/test/LayerZeroEndpointMock.sol @@ -3,10 +3,23 @@ pragma solidity >=0.8.0; /** * @dev Placeholder contract to provide a non-zero endpoint address for OFT adapter constructor. - * Tests that revert before LayerZero calls do not require any endpoint logic. + * Tests that revert before LayerZero calls do not require any endpoint logic, beyond the compose + * dispatch a replayed composed message performs. */ contract LayerZeroEndpointMock { + /// @dev Records the compose payload the OFT dispatches for a composed message. + event ComposeSent(address indexed from, address indexed to, bytes32 indexed guid, uint16 index, bytes message); + + mapping(bytes32 => bytes) public composeQueue; + // OAppCoreUpgradeable expects endpoint.setDelegate(_delegate) during initialization. function setDelegate(address) external {} + + function sendCompose(address _to, bytes32 _guid, uint16 _index, bytes calldata _message) external { + bytes32 key = keccak256(abi.encodePacked(msg.sender, _to, _guid, _index)); + require(composeQueue[key].length == 0, "LZ_ComposeExists"); + composeQueue[key] = _message; + emit ComposeSent(msg.sender, _to, _guid, _index, _message); + } } diff --git a/packages/bridge-contracts/test/GoodDollarOFT.reentrancy.test.ts b/packages/bridge-contracts/test/GoodDollarOFT.reentrancy.test.ts index 104a7a4..41ca6fe 100644 --- a/packages/bridge-contracts/test/GoodDollarOFT.reentrancy.test.ts +++ b/packages/bridge-contracts/test/GoodDollarOFT.reentrancy.test.ts @@ -53,6 +53,7 @@ async function assertReentrancyBlocked(harness: any, token: any) { await networkHelpers.time.increase(OPTIMISTIC_WINDOW + 1); + // approval now replays the parked message, so the hook fires inside the replay rather than inside _credit await expect(attacker.attack(guid, 30)).to.be.revertedWith('ReentrancyGuard: reentrant call'); expect(await token.balanceOf(attacker.address)).to.equal(0); expect((await harness.failedReceiveRequests(guid)).failed).to.equal(true); diff --git a/packages/bridge-contracts/test/GoodDollarOFT.test.ts b/packages/bridge-contracts/test/GoodDollarOFT.test.ts index 7772d4a..02a923d 100644 --- a/packages/bridge-contracts/test/GoodDollarOFT.test.ts +++ b/packages/bridge-contracts/test/GoodDollarOFT.test.ts @@ -15,6 +15,7 @@ function encodeOFTMessage(toAddress: string, amountSD: ethers.BigNumber): string // Convert local-decimal amount to shared-decimal amount (sharedDecimals=6) // For MockGoodDollar (18 decimals): decimalConversionRate = 10^(18-6) = 10^12 const DECIMAL_CONVERSION_RATE = ethers.BigNumber.from(10).pow(12); + function toSD(amountLD: ethers.BigNumber): ethers.BigNumber { return amountLD.div(DECIMAL_CONVERSION_RATE); } @@ -88,7 +89,7 @@ describe("OFT (unit, no fork)", () => { .withArgs(adapter.address, true); }); - it("operators can mint/burn; burn requires allowance", async () => { + it("operators can mint/burn; burn requires the tokens to be delivered first", async () => { const { minterBurner, token, user, avatar, owner } = await loadFixture(fixture); await expect(minterBurner.connect(user).mint(user.address, 1)).to.be.revertedWith("Not authorized"); @@ -103,18 +104,20 @@ describe("OFT (unit, no fork)", () => { "TokensMinted" ); - await expect(minterBurner.connect(owner).burn(owner.address, amount)).to.be.revertedWith( - "ERC20: insufficient allowance" + // burn only ever debits the MinterBurner itself, so an allowance no longer helps + await token.connect(owner).approve(minterBurner.address, amount); + await expect(minterBurner.connect(owner).burn(minterBurner.address, amount)).to.be.revertedWith( + "tokens not received" ); - await token.connect(owner).approve(minterBurner.address, amount); + await token.connect(owner).transfer(minterBurner.address, amount); const supplyBefore = await token.totalSupply(); - await expect(minterBurner.connect(owner).burn(owner.address, amount)).to.emit( - minterBurner, - "TokensBurned" - ); + await expect(minterBurner.connect(owner).burn(minterBurner.address, amount)) + .to.emit(minterBurner, "TokensBurned") + .withArgs(minterBurner.address, amount, owner.address); expect(await token.totalSupply()).to.equal(supplyBefore.sub(amount)); + expect(await token.balanceOf(minterBurner.address)).to.equal(0); }); // L-01: an operator can no longer burn tokens of an arbitrary approver @@ -130,6 +133,14 @@ describe("OFT (unit, no fork)", () => { "can only burn own tokens" ); expect(await token.balanceOf(user.address)).to.equal(amount); + + // still refused once the MinterBurner holds a burnable balance of its own, so no operator can + // launder a third party's allowance through it + await minterBurner.connect(owner).mint(minterBurner.address, amount); + await expect(minterBurner.connect(owner).burn(user.address, amount)).to.be.revertedWith( + "can only burn own tokens" + ); + expect(await token.balanceOf(user.address)).to.equal(amount); }); it("pause blocks operator mint/burn", async () => { @@ -142,7 +153,6 @@ describe("OFT (unit, no fork)", () => { "Contract is paused" ); - await token.connect(owner).approve(minterBurner.address, 1); await expect(minterBurner.connect(owner).burn(owner.address, 1)).to.be.revertedWith( "Contract is paused" ); @@ -626,12 +636,9 @@ describe("OFT (unit, no fork)", () => { return { owner, user, feeRecipient, avatar, nameService, token, endpoint, minterBurner, harness }; } - // Helper: build an Origin struct for testing - const origin = (srcEid = 1) => ({ - srcEid, - sender: ethers.utils.hexZeroPad(ethers.constants.AddressZero, 32), - nonce: 1, - }); + // Helper: build an Origin struct for testing. A non-zero sender/nonce keeps the parked Origin meaningful. + const PEER = ethers.utils.hexZeroPad("0x00000000000000000000000000000000000000aa", 32); + const origin = (srcEid = 1, nonce = 1) => ({ srcEid, sender: PEER, nonce }); it("stores failedReceiveRequest when bridge limits exceeded on receive", async () => { const { harness, user } = await loadFixture(harnessFixture); @@ -652,11 +659,65 @@ describe("OFT (unit, no fork)", () => { .to.emit(harness, "ReceiveRequestFailed") .withArgs(guid, user.address, amountLD, 1); + // only what the replay needs is parked: the message plus the Origin to replay it under. + // recipient and amount are not stored - the event above carries them, and they are derivable + // from the message anyway const req = await harness.failedReceiveRequests(guid); expect(req.failed).to.equal(true); - expect(req.toAddress).to.equal(user.address); - expect(req.amount).to.equal(amountLD); + expect(req.message).to.equal(message.toLowerCase()); expect(req.srcEid).to.equal(1); + expect(req.nonce).to.equal(1); + expect(req.sender).to.equal(PEER); + expect(req.timestamp).to.be.gt(0); + + // the replay still credits the right recipient the right amount, decoded from the parked message + await expect(harness.approveFailedRequest(guid)) + .to.emit(harness, "OFTReceived") + .withArgs(guid, 1, user.address, amountLD); + }); + + it("failedRequestDetails decodes the recipient and amount the struct no longer stores", async () => { + const { harness, user } = await loadFixture(harnessFixture); + await harness.setBridgeLimits({ + dailyLimit: ethers.utils.parseEther("1000"), + txLimit: ethers.utils.parseEther("50"), + accountDailyLimit: ethers.utils.parseEther("1000"), + minAmount: ethers.utils.parseEther("1"), + onlyWhitelisted: false, + }); + + const amountLD = ethers.utils.parseEther("100"); + const guid = ethers.utils.formatBytes32String("guid-details"); + const message = encodeOFTMessage(user.address, toSD(amountLD)); + await harness.exposed_lzReceive(origin(), guid, message, ethers.constants.AddressZero, "0x"); + + const plain = await harness.failedRequestDetails(guid); + expect(plain.toAddress).to.equal(user.address); + expect(plain.amountLD).to.equal(amountLD); + expect(plain.isComposed).to.equal(false); + + // and it reports a composed payload, so a caller can tell a compose will be dispatched on approval + const composedGuid = ethers.utils.formatBytes32String("guid-details-composed"); + const composedMessage = + message + ethers.utils.hexZeroPad(user.address, 32).slice(2) + "deadbeef"; + await harness.exposed_lzReceive(origin(1, 2), composedGuid, composedMessage, ethers.constants.AddressZero, "0x"); + + const composed = await harness.failedRequestDetails(composedGuid); + expect(composed.toAddress).to.equal(user.address); + expect(composed.amountLD).to.equal(amountLD); + expect(composed.isComposed).to.equal(true); + + // the raw message stays readable straight off the autogenerated getter + expect((await harness.failedReceiveRequests(composedGuid)).message).to.equal( + composedMessage.toLowerCase() + ); + }); + + it("failedRequestDetails reverts for a guid with no parked message", async () => { + const { harness } = await loadFixture(harnessFixture); + await expect( + harness.failedRequestDetails(ethers.utils.formatBytes32String("guid-absent")) + ).to.be.revertedWith("no parked message"); }); it("reverts with BRIDGE_NOT_ALLOWED when bridge is closed on receive", async () => { @@ -714,7 +775,7 @@ describe("OFT (unit, no fork)", () => { await harness.exposed_lzReceive(origin(), guid, message, ethers.constants.AddressZero, "0x"); // Confirm it's stored - expect((await harness.failedReceiveRequests(guid)).failed).to.be.true; + expect((await harness.failedReceiveRequests(guid)).failed).to.equal(true); // Raise limits so _credit succeeds await harness.setBridgeLimits({ @@ -732,7 +793,7 @@ describe("OFT (unit, no fork)", () => { .to.emit(harness, "FailedReceiveRequestApproved") .withArgs(guid); - expect((await harness.failedReceiveRequests(guid)).failed).to.be.false; + expect((await harness.failedReceiveRequests(guid)).failed).to.equal(false); expect((await token.balanceOf(user.address)).sub(balBefore)).to.equal(amountLD); }); @@ -790,7 +851,7 @@ describe("OFT (unit, no fork)", () => { const guid = ethers.utils.formatBytes32String(label); const message = encodeOFTMessage(to, toSD(ethers.utils.parseEther("100"))); await harness.exposed_lzReceive(origin(), guid, message, ethers.constants.AddressZero, "0x"); - expect((await harness.failedReceiveRequests(guid)).failed).to.be.true; + expect((await harness.failedReceiveRequests(guid)).failed).to.equal(true); return guid; } @@ -817,7 +878,7 @@ describe("OFT (unit, no fork)", () => { await expect(harness.rejectFailedRequest(guid)) .to.emit(harness, "FailedReceiveRequestRejected") .withArgs(guid); - expect((await harness.failedReceiveRequests(guid)).failed).to.be.false; + expect((await harness.failedReceiveRequests(guid)).failed).to.equal(false); await time.increase(3 * 24 * 60 * 60 + 1); await expect(harness.connect(user).approveFailedRequest(guid)).to.be.revertedWith("request not failed"); @@ -830,7 +891,7 @@ describe("OFT (unit, no fork)", () => { const guid = await parkFailedRequest(harness, user.address, "guid-reject-non-owner"); await expect(harness.connect(user).rejectFailedRequest(guid)).to.be.revertedWith("Ownable: caller is not the owner"); - expect((await harness.failedReceiveRequests(guid)).failed).to.be.true; + expect((await harness.failedReceiveRequests(guid)).failed).to.equal(true); }); it("rejectFailedRequest: reverts for non-existent or already approved request", async () => { @@ -866,9 +927,9 @@ describe("OFT (unit, no fork)", () => { expect(await token.balanceOf(user.address)).to.equal(netAmount); }); - // M-02: composed messages must not be parked (the compose payload would be lost) - it("reverts composed messages that exceed limits instead of parking them", async () => { - const { harness, user } = await loadFixture(harnessFixture); + // M-02: a composed message parks like any other, and approval replays it compose payload included + it("parks a composed message that exceeds limits and dispatches the compose on approval", async () => { + const { harness, endpoint, user, token } = await loadFixture(harnessFixture); await harness.setBridgeLimits({ dailyLimit: ethers.utils.parseEther("1000"), txLimit: ethers.utils.parseEther("50"), @@ -877,15 +938,46 @@ describe("OFT (unit, no fork)", () => { onlyWhitelisted: false, }); + const amountLD = ethers.utils.parseEther("100"); const guid = ethers.utils.formatBytes32String("guid-composed"); const composeFrom = ethers.utils.hexZeroPad(user.address, 32); - const message = - encodeOFTMessage(user.address, toSD(ethers.utils.parseEther("100"))) + composeFrom.slice(2) + "deadbeef"; + const message = encodeOFTMessage(user.address, toSD(amountLD)) + composeFrom.slice(2) + "deadbeef"; await expect(harness.exposed_lzReceive(origin(), guid, message, ethers.constants.AddressZero, "0x")) - .to.be.revertedWithCustomError(harness, "BRIDGE_LIMITS") - .withArgs("txLimit"); - expect((await harness.failedReceiveRequests(guid)).failed).to.equal(false); + .to.emit(harness, "ReceiveRequestFailed") + .withArgs(guid, user.address, amountLD, 1); + + const req = await harness.failedReceiveRequests(guid); + expect(req.failed).to.equal(true); + expect(req.message).to.equal(message.toLowerCase()); + + // the replay goes through the full OFT receive path, so the compose payload is dispatched + // rather than silently dropped the way a credit-only approval would + await expect(harness.approveFailedRequest(guid)) + .to.emit(endpoint, "ComposeSent") + .withArgs(harness.address, user.address, guid, 0, (composed: string) => composed.endsWith("deadbeef")); + expect(await token.balanceOf(user.address)).to.equal(amountLD); + }); + + it("an approved replay emits OFTReceived, so it is indistinguishable from a normal receive", async () => { + const { harness, user } = await loadFixture(harnessFixture); + const guid = await parkFailedRequest(harness, user.address, "guid-replay-event"); + + await expect(harness.approveFailedRequest(guid)) + .to.emit(harness, "OFTReceived") + .withArgs(guid, 1, user.address, ethers.utils.parseEther("100")); + }); + + it("replayFailedRequest cannot be called from outside", async () => { + const { harness, user } = await loadFixture(harnessFixture); + const message = encodeOFTMessage(user.address, toSD(ethers.utils.parseEther("100"))); + + await expect( + harness.replayFailedRequest(origin(), ethers.utils.formatBytes32String("guid-direct"), message) + ).to.be.revertedWith("only self"); + await expect( + harness.connect(user).replayFailedRequest(origin(), ethers.utils.formatBytes32String("guid-direct"), message) + ).to.be.revertedWith("only self"); }); // M-03: inbound and outbound traffic use separate counters @@ -1124,21 +1216,26 @@ describe("OFT (unit, no fork)", () => { return { user, token, harness, minterBurner }; } - it("pulls approved tokens into the adapter and burns them", async () => { + it("moves approved tokens straight to the MinterBurner and burns them, attributed to the sender", async () => { const { user, token, harness, minterBurner } = await loadFixture(debitFixture); const amount = ethers.utils.parseEther("10"); await token.mint(user.address, amount); await token.connect(user).approve(harness.address, amount); const supplyBefore = await token.totalSupply(); + // the burn debits the MinterBurner; SenderTokensBurned is what ties it to the user await expect(harness.exposed_debit(user.address, amount, amount, 1)) .to.emit(minterBurner, "TokensBurned") - .withArgs(harness.address, amount, harness.address); + .withArgs(minterBurner.address, amount, harness.address) + .and.to.emit(harness, "SenderTokensBurned") + .withArgs(user.address, amount); expect(await token.balanceOf(user.address)).to.equal(0); + // the adapter never takes custody on the way through expect(await token.balanceOf(harness.address)).to.equal(0); expect(await token.balanceOf(minterBurner.address)).to.equal(0); expect(await token.totalSupply()).to.equal(supplyBefore.sub(amount)); + // and never needs an allowance to the MinterBurner expect(await token.allowance(harness.address, minterBurner.address)).to.equal(0); });