Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ETHEREUM-CONTRACTS] add missing batch call operations #1967

Merged
merged 38 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
c163b35
added batch call operations for upgradeTo and downgradeTo
d10r Jun 24, 2024
947699e
added test case for connecting a pool via batch call
d10r Jun 24, 2024
84bd23e
added OPERATION_TYPE_SIMPLE_FORWARD_CALL
d10r Jun 24, 2024
1f31d3c
added OPERATION_TYPE_ERC2771_FORWARD_CALL
d10r Jun 25, 2024
b9582cd
forward native tokens with the first generic external call
d10r Jun 27, 2024
a1ae6ad
fix contract size
d10r Jun 27, 2024
8152677
appease linter
d10r Jun 27, 2024
b78a0d3
more comments, fixed value forwarding via 2771
d10r Jun 27, 2024
839072c
more verbose documentation
d10r Jun 27, 2024
1391b1f
fix hotfuzz init
d10r Jun 27, 2024
23048b0
add dmzfwd to deploy script
d10r Jun 27, 2024
48279c1
adjust mock constructor
d10r Jun 27, 2024
71b0158
disable code to stay inside contract size limit
d10r Jun 27, 2024
30aed68
this is insanity
d10r Jun 27, 2024
9ec13fb
fix test
d10r Jun 28, 2024
39dfb01
fix deployment test
d10r Jun 28, 2024
34c5bb5
disable only part of the replacePlaceholderCtx test
d10r Jun 28, 2024
804734d
revert unrelated change
d10r Jun 28, 2024
5772a07
try failing test only
d10r Jun 28, 2024
ea4d3a3
fixed the flakiness source and undid the undo
d10r Jun 28, 2024
bb6fda4
updated CHANGELOG
d10r Jun 28, 2024
08a17fd
allow to withdraw native tokens stuck in DMZForwarder
d10r Jul 1, 2024
b511690
made host.forwardBatchCall payable too
d10r Jul 1, 2024
3f4c8e3
rename to GeneralDistributionAgreementV1.prop.t.sol
hellwolf Jul 1, 2024
913d39d
update foundry
hellwolf Jul 1, 2024
43176b0
use assembly destructor to have different error code
hellwolf Jul 1, 2024
0cd9fc3
uncomment out test code
hellwolf Jul 1, 2024
08ffbc3
update foundry version
hellwolf Jul 1, 2024
77dae62
[ETHEREUM-CONTRACTS]: typo fixes
hellwolf Jul 1, 2024
5d7011b
[ETHEREUM-CONTRACTS] rename prop.sol to prop.t.sol
hellwolf Jul 1, 2024
fb2654a
fix build warning
hellwolf Jul 1, 2024
4833be0
ignore mocks from hardhat build
hellwolf Jul 1, 2024
d8b9e17
use assembly selfdestruct for now
hellwolf Jul 1, 2024
287e1c2
fix build warning
hellwolf Jul 1, 2024
5fceaf1
update CHANGELOG
hellwolf Jul 1, 2024
84bb8ba
Merge branch 'assorted-test-improvements-20240701' into batch-ops
hellwolf Jul 1, 2024
4daae4b
Merge branch 'dev' into batch-ops
hellwolf Jul 1, 2024
524ae94
Merge branch 'dev' into batch-ops
hellwolf Jul 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions packages/ethereum-contracts/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,16 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

## [unreleased]

### Added

- `batchCall` now supports 4 additional operation types:
- `OPERATION_TYPE_SUPERTOKEN_UPGRADE_TO`
- `OPERATION_TYPE_SUPERTOKEN_DOWNGRADE_TO`
- `OPERATION_TYPE_SIMPLE_FORWARD_CALL`
- `OPERATION_TYPE_ERC2771_FORWARD_CALL`

The latter 2 allow to add arbitrary contract calls to batch call.

### Changed

- fix a few types and build warnings
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,24 @@ library BatchOperation {
* )
*/
uint32 constant internal OPERATION_TYPE_SUPERTOKEN_DOWNGRADE = 2 + 100;
/**
* @dev SuperToken.upgradeTo batch operation type
*
* Call spec:
* ISuperToken(target).operationUpgradeTo(
* abi.decode(data, (address to, uint256 amount)
* )
*/
uint32 constant internal OPERATION_TYPE_SUPERTOKEN_UPGRADE_TO = 3 + 100;
/**
* @dev SuperToken.downgradeTo batch operation type
*
* Call spec:
* ISuperToken(target).operationDowngradeTo(
* abi.decode(data, (address to, uint256 amount)
* )
*/
uint32 constant internal OPERATION_TYPE_SUPERTOKEN_DOWNGRADE_TO = 4 + 100;
/**
* @dev Superfluid.callAgreement batch operation type
*
Expand All @@ -212,6 +230,41 @@ library BatchOperation {
* )
*/
uint32 constant internal OPERATION_TYPE_SUPERFLUID_CALL_APP_ACTION = 2 + 200;
/**
* @dev DMZForwarder.forwardCall batch operation type
*
* Call spec:
* forwardCall(
* target,
* data
* )
*/
uint32 constant internal OPERATION_TYPE_SIMPLE_FORWARD_CALL = 1 + 300;
/**
* @dev DMZForwarder.forward2771Call batch operation type
*
* Call spec:
* forward2771Call(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be in sync with the rest, use typed signature, and call "data" "callData" to be more specific.

* target,
* msgSender,
* data
* )
*
* NOTE: In the context of this operation, the `DZMForwarder` contract acts as the
* _trusted forwarder_ which must be trusted by the _recipient contract_ (operation target).
* It shall do so by dynamically looking up the DMZForwarder used by the host, like this:
*
* function isTrustedForwarder(address forwarder) public view returns(bool) {
* return forwarder == address(host.DMZ_FORWARDER());
* }
*
* If used in the context of a `forwardBatchCall`, we effectively have a chaining/nesting
* of ERC-2771 calls where the host acts as _recipient contract_ of the enveloping 2771 call
* and the DMZForwarder acts as the _trusted forwarder_ of the nested 2771 call(s).
* That's why `msgSender` could be either the actual `msg.sender` (if using `batchCall`)
* or the relayed sender address (if using `forwardBatchCall`).
*/
uint32 constant internal OPERATION_TYPE_ERC2771_FORWARD_CALL = 2 + 300;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,28 @@ interface ISuperToken is ISuperfluidToken, IERC20Metadata, IERC777 {
*/
function operationDowngrade(address account, uint256 amount) external;

/**
* @dev Upgrade ERC20 to SuperToken by host contract and transfer immediately.
* @param account The account to be changed.
* @param to The account to receive upgraded tokens
* @param amount Number of tokens to be upgraded (in 18 decimals)
*
* @custom:modifiers
* - onlyHost
*/
function operationUpgradeTo(address account, address to, uint256 amount) external;

/**
* @dev Downgrade ERC20 to SuperToken by host contract and transfer immediately.
* @param account The account to be changed.
* @param to The account to receive downgraded tokens
* @param amount Number of tokens to be downgraded (in 18 decimals)
*
* @custom:modifiers
* - onlyHost
*/
function operationDowngradeTo(address account, address to, uint256 amount) external;

// Flow NFT events
/**
* @dev Constant Outflow NFT proxy created event
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -622,14 +622,31 @@ interface ISuperfluid {
/**
* @dev Batch call function
* @param operations Array of batch operations
*
* NOTE: `batchCall` is `payable, because there's limited support for sending
* native tokens to batch operation targets.
* If value is > 0, the whole amount is sent to the first operation matching any of:
* - OPERATION_TYPE_SUPERFLUID_CALL_APP_ACTION
* - OPERATION_TYPE_SIMPLE_FORWARD_CALL
* - OPERATION_TYPE_ERC2771_FORWARD_CALL
* If the first such operation does not allow receiving native tokens,
* the transaction will revert.
* It's currently not possible to send native tokens to multiple operations, or to
* any but the first operation of one of the above mentioned types.
* If no such operation is included, the native tokens will be sent back to the sender.
*/
function batchCall(Operation[] calldata operations) external payable;

/**
* @dev Batch call function for trusted forwarders (EIP-2771)
* @dev Batch call function with EIP-2771 encoded msgSender
* @param operations Array of batch operations
*
* NOTE: This can be called only by contracts recognized as _trusted forwarder_
* by the host contract (see `Superfluid.isTrustedForwarder`).
* If native tokens are passed along, the same rules as for `batchCall` apply,
* with an optional refund going to the encoded msgSender.
*/
function forwardBatchCall(Operation[] calldata operations) external;
function forwardBatchCall(Operation[] calldata operations) external payable;

/**************************************************************************
* Function modifiers for access control and parameter validations
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { CallUtils } from "../libs/CallUtils.sol";

contract SuperfluidUpgradabilityTester is Superfluid {

constructor() Superfluid(false, false)
constructor() Superfluid(false, false, address(0))
// solhint-disable-next-line no-empty-blocks
{
}
Expand Down Expand Up @@ -129,9 +129,8 @@ contract SuperfluidUpgradabilityTester is Superfluid {

contract SuperfluidMock is Superfluid {


constructor(bool nonUpgradable, bool appWhiteListingEnabled)
Superfluid(nonUpgradable, appWhiteListingEnabled)
constructor(bool nonUpgradable, bool appWhiteListingEnabled, address dmzForwarder)
Superfluid(nonUpgradable, appWhiteListingEnabled, dmzForwarder)
// solhint-disable-next-line no-empty-blocks
{
}
Expand Down Expand Up @@ -173,5 +172,4 @@ contract SuperfluidMock is Superfluid {
{
_jailApp(app, 6942);
}

}
14 changes: 14 additions & 0 deletions packages/ethereum-contracts/contracts/superfluid/SuperToken.sol
Original file line number Diff line number Diff line change
Expand Up @@ -882,6 +882,20 @@ contract SuperToken is
_downgrade(msg.sender, account, account, amount, "", "");
}

function operationUpgradeTo(address account, address to, uint256 amount)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will require Token upgrades

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right. Also we may later consider deprecating the variants without to argument.

external virtual override
onlyHost
{
_upgrade(msg.sender, account, to, amount, "", "");
}

function operationDowngradeTo(address account, address to, uint256 amount)
external virtual override
onlyHost
{
_downgrade(msg.sender, account, to, amount, "", "");
}

/**************************************************************************
* Modifiers
*************************************************************************/
Expand Down
63 changes: 50 additions & 13 deletions packages/ethereum-contracts/contracts/superfluid/Superfluid.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { GeneralDistributionAgreementV1 } from "../agreements/gdav1/GeneralDistr
import { SuperfluidUpgradeableBeacon } from "../upgradability/SuperfluidUpgradeableBeacon.sol";
import { CallUtils } from "../libs/CallUtils.sol";
import { BaseRelayRecipient } from "../libs/BaseRelayRecipient.sol";
import { DMZForwarder } from "../utils/DMZForwarder.sol";

/**
* @dev The Superfluid host implementation.
Expand Down Expand Up @@ -51,6 +52,8 @@ contract Superfluid is
// solhint-disable-next-line var-name-mixedcase
bool immutable public APP_WHITE_LISTING_ENABLED;

DMZForwarder immutable public DMZ_FORWARDER;

/**
* @dev Maximum number of level of apps can be composed together
*
Expand Down Expand Up @@ -95,9 +98,10 @@ contract Superfluid is
/// function in its respective mock contract to ensure that it doesn't break anything or lead to unexpected
/// behaviors/layout when upgrading

constructor(bool nonUpgradable, bool appWhiteListingEnabled) {
constructor(bool nonUpgradable, bool appWhiteListingEnabled, address dmzForwarderAddress) {
NON_UPGRADABLE_DEPLOYMENT = nonUpgradable;
APP_WHITE_LISTING_ENABLED = appWhiteListingEnabled;
DMZ_FORWARDER = DMZForwarder(dmzForwarderAddress);
}

////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -312,7 +316,7 @@ contract Superfluid is
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
// Superfluid Upgradeable Beacon
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

/// @inheritdoc ISuperfluid
function updatePoolBeaconLogic(address newLogic) external override onlyGovernance {
GeneralDistributionAgreementV1 gda = GeneralDistributionAgreementV1(
Expand Down Expand Up @@ -794,12 +798,11 @@ contract Superfluid is
**************************************************************************/

function _batchCall(
address msgSender,
address payable msgSender,
Operation[] calldata operations
)
internal
{
bool valueForwarded = false;
for (uint256 i = 0; i < operations.length; ++i) {
uint32 operationType = operations[i].operationType;
if (operationType == BatchOperation.OPERATION_TYPE_ERC20_APPROVE) {
Expand Down Expand Up @@ -847,27 +850,61 @@ contract Superfluid is
ISuperToken(operations[i].target).operationDowngrade(
msgSender,
abi.decode(operations[i].data, (uint256))); // amount
} else if (operationType == BatchOperation.OPERATION_TYPE_SUPERTOKEN_UPGRADE_TO) {
(address to, uint256 amount) = abi.decode(operations[i].data, (address, uint256));
ISuperToken(operations[i].target).operationUpgradeTo(
msgSender,
to,
amount);
} else if (operationType == BatchOperation.OPERATION_TYPE_SUPERTOKEN_DOWNGRADE_TO) {
(address to, uint256 amount) = abi.decode(operations[i].data, (address, uint256));
ISuperToken(operations[i].target).operationDowngradeTo(
msgSender,
to,
amount);
} else if (operationType == BatchOperation.OPERATION_TYPE_SUPERFLUID_CALL_AGREEMENT) {
(bytes memory callData, bytes memory userData) = abi.decode(operations[i].data, (bytes, bytes));
_callAgreement(
msgSender,
ISuperAgreement(operations[i].target),
callData,
userData);
} else if (operationType == BatchOperation.OPERATION_TYPE_SUPERFLUID_CALL_APP_ACTION) {
}
// The following operations for call proxies allow forwarding of native tokens.
// we use `address(this).balance` instead of `msg.value`, because the latter ist not
// updated after forwarding to the first operation, while `balance` is.
// The initial balance is equal to `msg.value` because there's no other path
// for the contract to receive native tokens.
else if (operationType == BatchOperation.OPERATION_TYPE_SUPERFLUID_CALL_APP_ACTION) {
_callAppAction(
msgSender,
ISuperApp(operations[i].target),
valueForwarded ? 0 : msg.value,
address(this).balance,
operations[i].data);
valueForwarded = true;
} else if (operationType == BatchOperation.OPERATION_TYPE_SIMPLE_FORWARD_CALL) {
(bool success, bytes memory returnData) =
DMZ_FORWARDER.forwardCall{value: address(this).balance}(
operations[i].target,
operations[i].data);
if (!success) {
CallUtils.revertFromReturnedData(returnData);
}
} else if (operationType == BatchOperation.OPERATION_TYPE_ERC2771_FORWARD_CALL) {
(bool success, bytes memory returnData) =
DMZ_FORWARDER.forward2771Call{value: address(this).balance}(
operations[i].target,
msgSender,
operations[i].data);
if (!success) {
CallUtils.revertFromReturnedData(returnData);
}
} else {
revert HOST_UNKNOWN_BATCH_CALL_OPERATION_TYPE();
}
}
if (msg.value != 0 && !valueForwarded) {
// return ETH provided if not forwarded
payable(msg.sender).transfer(msg.value);
if (address(this).balance != 0) {
// return any native tokens left to the sender.
msgSender.transfer(address(this).balance);
}
}

Expand All @@ -877,12 +914,12 @@ contract Superfluid is
)
external override payable
{
_batchCall(msg.sender, operations);
_batchCall(payable(msg.sender), operations);
}

/// @dev ISuperfluid.forwardBatchCall implementation
function forwardBatchCall(Operation[] calldata operations)
external override
external override payable
{
_batchCall(_getTransactionSigner(), operations);
}
Expand All @@ -899,7 +936,7 @@ contract Superfluid is
) != 0;
}

/// @dev IRelayRecipient.isTrustedForwarder implementation
/// @dev IRelayRecipient.versionRecipient implementation
function versionRecipient()
external override pure
returns (string memory)
Expand Down
50 changes: 50 additions & 0 deletions packages/ethereum-contracts/contracts/utils/DMZForwarder.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// SPDX-License-Identifier: AGPLv3
pragma solidity 0.8.23;

import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";

/**
* @title DMZForwarder
* @dev The purpose of this contract is to make arbitrary contract calls batchable
* alongside Superfluid specific batch operations.
* We route the calls through this dedicated contract in order to not have msg.sender set
* to the host contract, for security reasons.
* Forwarded calls can optionally use ERC-2771 to preserve the original msg.sender.
* If native tokens (msg.value) are provided, they are forwarded as well.
*/
contract DMZForwarder is Ownable {
/**
* @dev Forwards a call for which msg.sender doesn't matter
* @param target The target contract to call
* @param data The call data
*/
function forwardCall(address target, bytes memory data)
external payable
returns(bool success, bytes memory returnData)
{
// solhint-disable-next-line avoid-low-level-calls
(success, returnData) = target.call{value: msg.value}(data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any merit to using address(this).balance here or trying to return the leftover? (Probably not.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any merit to using address(this).balance here

I don't think so

trying to return the leftover

No, but I added a function for withdrawing stuck native tokens.

}

/**
* @dev Forwards a call passing along the original msg.sender encoded as specified in ERC-2771.
* @param target The target contract to call
* @param msgSender The original msg.sender passed along by the trusted contract owner
* @param data The call data
*/
function forward2771Call(address target, address msgSender, bytes memory data)
external payable onlyOwner
returns(bool success, bytes memory returnData)
{
// solhint-disable-next-line avoid-low-level-calls
(success, returnData) = target.call{value: msg.value}(abi.encodePacked(data, msgSender));
}

/**
* @dev Allows to withdraw native tokens (ETH) which got stuck in this contract.
* This could happen if a call fails, but the caller doesn't revert the tx.
*/
function withdrawLostNativeTokens(address payable receiver) external onlyOwner {
receiver.transfer(address(this).balance);
}
}
Loading
Loading