Fix/oft bridge followups

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,17 +304,64 @@ 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 {
         require(!isClosed, 'closed');
         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 {}

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;

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

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