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

feat: Develop SingleRequestProxy Smart Contracts #1453

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e2567f4
feat: develop for native currencies
aimensahnoun Sep 12, 2024
c624474
feat: develop for ERC20 currencies
aimensahnoun Sep 12, 2024
917d472
feat: create `EthereumSingleRequestProxy` factory contract
aimensahnoun Sep 12, 2024
d5e4728
feat: create `ERC20SingleRequestProxy` factory contract
aimensahnoun Sep 12, 2024
0543092
refactor: merge the single request proxy factories into one factory
aimensahnoun Sep 13, 2024
2bd3495
fix: infinite loop between `EthSingleRequestProxy` and `EthFeeProxy`
aimensahnoun Sep 13, 2024
19b5e40
refactor: refactor `nonReentrant` to match openzeppelin implementation
aimensahnoun Sep 16, 2024
c1d3a77
fix: update `ERC20SingleRequestProxy` to use `balance`
aimensahnoun Sep 16, 2024
1376582
fix: update `SingleRequestProxyFactory` to call Ownable constructor
aimensahnoun Sep 16, 2024
c2f55ee
chore: update contracts to use `0.8.9` solidity version
aimensahnoun Sep 17, 2024
bc922e8
test: add test for `EthereumSingleRequestProxy`
aimensahnoun Sep 18, 2024
8aef234
fix: `ERC20SingleRequestProxy` to factor in fee amount
aimensahnoun Sep 18, 2024
0769aa7
test: `ERC20SingleRequestProxy` functionality
aimensahnoun Sep 18, 2024
3bd9e38
test: `SingleRequestProxyFactory` functionality
aimensahnoun Sep 18, 2024
ed429ae
chore: update `EthereumSingleRequestProxy` tests
aimensahnoun Sep 18, 2024
d9a4484
Merge branch 'master' into 1394-develop-smart-contracts-nativesingler…
aimensahnoun Sep 18, 2024
a341ce1
feat: write deployment script for `SingleRequestProxyFactory`
aimensahnoun Sep 23, 2024
10974b5
Merge branch '1394-develop-smart-contracts-nativesinglerequestproxy-e…
aimensahnoun Sep 23, 2024
2cc82a0
refactor: rewrite deployment to use `CREATE2` schema
aimensahnoun Sep 23, 2024
bee1f58
fix: add SingleRequestProxyFactory `create2ContractDeploymentList`
aimensahnoun Sep 23, 2024
aab6711
chore: include SingleRequestProxyFactory in transfer ownership cases …
aimensahnoun Sep 23, 2024
0c68eba
refactor: rename updateEthereumFeeProxy to setEthereumFeeProxy for cl…
aimensahnoun Sep 23, 2024
796f2b9
Merge branch 'master' into 1394-develop-smart-contracts-nativesingler…
aimensahnoun Sep 23, 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
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ export const computeCreate2DeploymentAddressesFromList = async (
case 'BatchConversionPayments':
case 'ERC20SwapToPay':
case 'ERC20SwapToConversion':
case 'ERC20TransferableReceivable': {
case 'ERC20TransferableReceivable':
case 'SingleRequestProxyFactory': {
try {
const constructorArgs = getConstructorArgs(contract, chain);
address = await computeCreate2DeploymentAddress({ contract, constructorArgs }, hre);
Expand Down
11 changes: 11 additions & 0 deletions packages/smart-contracts/scripts-create2/constructor-args.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,17 @@ export const getConstructorArgs = (
const erc20FeeProxyAddress = erc20FeeProxy.getAddress(network);
return ['Request Network Transferable Receivable', 'tREC', erc20FeeProxyAddress];
}
case 'SingleRequestProxyFactory': {
if (!network) {
throw new Error('SingleRequestProxyFactory requires network parameter');
}
const erc20FeeProxy = artifacts.erc20FeeProxyArtifact;
const erc20FeeProxyAddress = erc20FeeProxy.getAddress(network);
const ethereumFeeProxy = artifacts.ethereumFeeProxyArtifact;
const ethereumFeeProxyAddress = ethereumFeeProxy.getAddress(network);

return [ethereumFeeProxyAddress, erc20FeeProxyAddress];
}
Comment on lines +70 to +80
Copy link

Choose a reason for hiding this comment

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

LGTM! Consider adding explanatory comments.

The implementation for the SingleRequestProxyFactory case is well-structured and consistent with other cases in the file. It correctly handles the network parameter requirement and retrieves the necessary addresses.

Consider adding a comment explaining the purpose of these addresses in the constructor, for example:

// Return addresses for ethereumFeeProxy and erc20FeeProxy, required for SingleRequestProxyFactory initialization
return [ethereumFeeProxyAddress, erc20FeeProxyAddress];

Also, please verify if the order of addresses in the return array is correct (ethereumFeeProxy first, then erc20FeeProxy) as intended for the SingleRequestProxyFactory constructor.

default:
return [];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ export const transferOwnership = async (
case 'Erc20ConversionProxy':
case 'BatchConversionPayments':
case 'ERC20SwapToPay':
case 'ERC20SwapToConversion': {
case 'ERC20SwapToConversion':
case 'SingleRequestProxyFactory': {
await updateOwner({ contract, hre, signWithEoa });
break;
}
Expand Down
1 change: 1 addition & 0 deletions packages/smart-contracts/scripts-create2/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const create2ContractDeploymentList = [
'BatchConversionPayments',
'ERC20EscrowToPay',
'ERC20TransferableReceivable',
'SingleRequestProxyFactory',
Copy link

Choose a reason for hiding this comment

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

Addition of 'SingleRequestProxyFactory' approved, but update required in getArtifact function

The addition of 'SingleRequestProxyFactory' to the create2ContractDeploymentList is consistent with the PR objectives. However, there's a potential issue that needs to be addressed.

The getArtifact function doesn't have a case for 'SingleRequestProxyFactory'. This omission will cause an error when attempting to deploy the contract. Please add the necessary case to the getArtifact function. For example:

case 'SingleRequestProxyFactory':
  return artifacts.singleRequestProxyFactoryArtifact;

Make sure to import the correct artifact from the artifacts file.

];

/**
Expand Down
57 changes: 57 additions & 0 deletions packages/smart-contracts/src/contracts/ERC20SingleRequestProxy.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.9;

import '@openzeppelin/contracts/token/ERC20/IERC20.sol';
import './interfaces/ERC20FeeProxy.sol';

/**
* @title ERC20SingleRequestProxy
* @notice This contract is used to send a single request to a payee with a fee sent to a third address for ERC20
*/

contract ERC20SingleRequestProxy {
address public payee;
address public tokenAddress;
address public feeAddress;
uint256 public feeAmount;
leoslr marked this conversation as resolved.
Show resolved Hide resolved
bytes public paymentReference;
IERC20FeeProxy public erc20FeeProxy;

constructor(
address _payee,
address _tokenAddress,
address _feeAddress,
uint256 _feeAmount,
bytes memory _paymentReference,
address _erc20FeeProxy
) {
payee = _payee;
tokenAddress = _tokenAddress;
feeAddress = _feeAddress;
feeAmount = _feeAmount;
paymentReference = _paymentReference;
erc20FeeProxy = IERC20FeeProxy(_erc20FeeProxy);
}
Comment on lines +20 to +34
Copy link

Choose a reason for hiding this comment

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

Add input validation in the constructor

Consider adding require statements to validate the constructor parameters. Ensuring that addresses are not the zero address helps prevent misconfigurations upon deployment.

Apply this diff to add input validation:

      ) {
+       require(_payee != address(0), 'Invalid payee address');
+       require(_tokenAddress != address(0), 'Invalid token address');
+       require(_erc20FeeProxy != address(0), 'Invalid ERC20FeeProxy address');

        payee = _payee;
        tokenAddress = _tokenAddress;
        feeAddress = _feeAddress;
        feeAmount = _feeAmount;
        paymentReference = _paymentReference;
        erc20FeeProxy = IERC20FeeProxy(_erc20FeeProxy);
      }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
constructor(
address _payee,
address _tokenAddress,
address _feeAddress,
uint256 _feeAmount,
bytes memory _paymentReference,
address _erc20FeeProxy
) {
payee = _payee;
tokenAddress = _tokenAddress;
feeAddress = _feeAddress;
feeAmount = _feeAmount;
paymentReference = _paymentReference;
erc20FeeProxy = IERC20FeeProxy(_erc20FeeProxy);
}
constructor(
address _payee,
address _tokenAddress,
address _feeAddress,
uint256 _feeAmount,
bytes memory _paymentReference,
address _erc20FeeProxy
) {
require(_payee != address(0), 'Invalid payee address');
require(_tokenAddress != address(0), 'Invalid token address');
require(_erc20FeeProxy != address(0), 'Invalid ERC20FeeProxy address');
payee = _payee;
tokenAddress = _tokenAddress;
feeAddress = _feeAddress;
feeAmount = _feeAmount;
paymentReference = _paymentReference;
erc20FeeProxy = IERC20FeeProxy(_erc20FeeProxy);
}


receive() external payable {
require(msg.value == 0, 'This function is only for triggering the transfer');
IERC20 token = IERC20(tokenAddress);
uint256 balance = token.balanceOf(address(this));
uint256 paymentAmount = balance;
if (feeAmount > 0 && feeAddress != address(0)) {
require(balance > feeAmount, 'Insufficient balance to cover fee');
paymentAmount = balance - feeAmount;
Comment on lines +42 to +43
Copy link

Choose a reason for hiding this comment

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

Adjust balance check to allow exact fee amount

The condition balance > feeAmount prevents execution when balance equals feeAmount. Change it to balance >= feeAmount to allow transactions where the balance exactly covers the fee.

Apply this diff to adjust the condition:

-         require(balance > feeAmount, 'Insufficient balance to cover fee');
+         require(balance >= feeAmount, 'Insufficient balance to cover fee');
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
require(balance > feeAmount, 'Insufficient balance to cover fee');
paymentAmount = balance - feeAmount;
require(balance >= feeAmount, 'Insufficient balance to cover fee');
paymentAmount = balance - feeAmount;

}

token.approve(address(erc20FeeProxy), balance);
Copy link

Choose a reason for hiding this comment

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

Limit token approval to the required amount

Approving the entire balance may pose a security risk if erc20FeeProxy is compromised. Approve only the exact amount needed for the transfer.

Apply this diff to limit the token approval:

-       token.approve(address(erc20FeeProxy), balance);
+       token.approve(address(erc20FeeProxy), paymentAmount + feeAmount);
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
token.approve(address(erc20FeeProxy), balance);
token.approve(address(erc20FeeProxy), paymentAmount + feeAmount);


erc20FeeProxy.transferFromWithReferenceAndFee(
tokenAddress,
payee,
paymentAmount,
paymentReference,
feeAmount,
feeAddress
);
}
Comment on lines +36 to +56
Copy link

Choose a reason for hiding this comment

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

Replace the receive function with a named executePayment function

Using the receive function to trigger token transfers can be confusing since it's typically meant for handling Ether transfers. A named function like executePayment() improves clarity and usability.

Apply this diff to replace the receive function:

-   receive() external payable {
+   function executePayment() external {
        require(msg.value == 0, 'This function is only for triggering the transfer');
        IERC20 token = IERC20(tokenAddress);
        uint256 balance = token.balanceOf(address(this));
        uint256 paymentAmount = balance;
        if (feeAmount > 0 && feeAddress != address(0)) {
-         require(balance > feeAmount, 'Insufficient balance to cover fee');
+         require(balance >= feeAmount, 'Insufficient balance to cover fee');
          paymentAmount = balance - feeAmount;
        }

        token.approve(address(erc20FeeProxy), balance);

        erc20FeeProxy.transferFromWithReferenceAndFee(
          tokenAddress,
          payee,
          paymentAmount,
          paymentReference,
          feeAmount,
          feeAddress
        );
      }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
receive() external payable {
require(msg.value == 0, 'This function is only for triggering the transfer');
IERC20 token = IERC20(tokenAddress);
uint256 balance = token.balanceOf(address(this));
uint256 paymentAmount = balance;
if (feeAmount > 0 && feeAddress != address(0)) {
require(balance > feeAmount, 'Insufficient balance to cover fee');
paymentAmount = balance - feeAmount;
}
token.approve(address(erc20FeeProxy), balance);
erc20FeeProxy.transferFromWithReferenceAndFee(
tokenAddress,
payee,
paymentAmount,
paymentReference,
feeAmount,
feeAddress
);
}
function executePayment() external {
require(msg.value == 0, 'This function is only for triggering the transfer');
IERC20 token = IERC20(tokenAddress);
uint256 balance = token.balanceOf(address(this));
uint256 paymentAmount = balance;
if (feeAmount > 0 && feeAddress != address(0)) {
require(balance >= feeAmount, 'Insufficient balance to cover fee');
paymentAmount = balance - feeAmount;
}
token.approve(address(erc20FeeProxy), balance);
erc20FeeProxy.transferFromWithReferenceAndFee(
tokenAddress,
payee,
paymentAmount,
paymentReference,
feeAmount,
feeAddress
);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.9;

import './interfaces/EthereumFeeProxy.sol';
Copy link

Choose a reason for hiding this comment

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

Verify the correct import path for IEthereumFeeProxy.

Ensure that the IEthereumFeeProxy interface is correctly imported. If the interface is defined in IEthereumFeeProxy.sol, consider updating the import statement to reflect the accurate file name.

Apply this diff if necessary:

- import './interfaces/EthereumFeeProxy.sol';
+ import './interfaces/IEthereumFeeProxy.sol';
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import './interfaces/EthereumFeeProxy.sol';
import './interfaces/IEthereumFeeProxy.sol';


/**
* @title EthereumSingleRequestProxy
* @notice This contract is used to send a single request to a payee with a fee sent to a third address
*/
contract EthereumSingleRequestProxy {
address public payee;
bytes public paymentReference;
address public feeAddress;
uint256 public feeAmount;
leoslr marked this conversation as resolved.
Show resolved Hide resolved
IEthereumFeeProxy public ethereumFeeProxy;
Comment on lines +11 to +15
Copy link

Choose a reason for hiding this comment

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

Restrict visibility of state variables if not needed publicly.

If external access to state variables is not required, consider changing their visibility to private or internal to encapsulate the contract's state.

Apply this diff if external access is unnecessary:

- address public payee;
- bytes public paymentReference;
- address public feeAddress;
- uint256 public feeAmount;
- IEthereumFeeProxy public ethereumFeeProxy;
+ address private payee;
+ bytes private paymentReference;
+ address private feeAddress;
+ uint256 private feeAmount;
+ IEthereumFeeProxy private ethereumFeeProxy;
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
address public payee;
bytes public paymentReference;
address public feeAddress;
uint256 public feeAmount;
IEthereumFeeProxy public ethereumFeeProxy;
address private payee;
bytes private paymentReference;
address private feeAddress;
uint256 private feeAmount;
IEthereumFeeProxy private ethereumFeeProxy;


address private originalSender;

// Reentrancy guard
uint256 private constant _NOT_ENTERED = 1;
uint256 private constant _ENTERED = 2;
uint256 private _status;
leoslr marked this conversation as resolved.
Show resolved Hide resolved

Comment on lines +19 to +23
Copy link

Choose a reason for hiding this comment

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

Use OpenZeppelin's ReentrancyGuard for reentrancy protection.

Implementing a custom reentrancy guard can be error-prone. It is recommended to inherit from OpenZeppelin's ReentrancyGuard contract to leverage a well-tested solution and simplify the code.

Apply this diff to use ReentrancyGuard:

+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

-contract EthereumSingleRequestProxy {
+contract EthereumSingleRequestProxy is ReentrancyGuard {

     address public payee;
     bytes public paymentReference;
     address public feeAddress;
     uint256 public feeAmount;
     IEthereumFeeProxy public ethereumFeeProxy;

-    address private originalSender;
-
-    // Reentrancy guard
-    uint256 private constant _NOT_ENTERED = 1;
-    uint256 private constant _ENTERED = 2;
-    uint256 private _status;

     constructor(
         address _payee,
         bytes memory _paymentReference,
         address _ethereumFeeProxy,
         address _feeAddress,
         uint256 _feeAmount
     ) {
         payee = _payee;
         paymentReference = _paymentReference;
         feeAddress = _feeAddress;
         feeAmount = _feeAmount;
         ethereumFeeProxy = IEthereumFeeProxy(_ethereumFeeProxy);
-        _status = _NOT_ENTERED;
     }

-    modifier nonReentrant() {
-        if (msg.sender != address(ethereumFeeProxy)) {
-            require(_status != _ENTERED, 'ReentrancyGuard: reentrant call');
-            _status = _ENTERED;
-        }
-        _;
-        if (msg.sender != address(ethereumFeeProxy)) {
-            _status = _NOT_ENTERED;
-        }
-    }
+    // Remove custom `nonReentrant` modifier and use the inherited one

Also applies to: 39-51

constructor(
address _payee,
bytes memory _paymentReference,
address _ethereumFeeProxy,
address _feeAddress,
uint256 _feeAmount
) {
payee = _payee;
paymentReference = _paymentReference;
feeAddress = _feeAddress;
feeAmount = _feeAmount;
ethereumFeeProxy = IEthereumFeeProxy(_ethereumFeeProxy);
_status = _NOT_ENTERED;
}

modifier nonReentrant() {
if (msg.sender != address(ethereumFeeProxy)) {
// On the first call to nonReentrant, _status will be _NOT_ENTERED
require(_status != _ENTERED, 'ReentrancyGuard: reentrant call');
// Any calls to nonReentrant after this point will fail
_status = _ENTERED;
}
_;
if (msg.sender != address(ethereumFeeProxy)) {
// By storing the original value once again, a refund is triggered
_status = _NOT_ENTERED;
}
}

receive() external payable nonReentrant {
if (msg.sender == address(ethereumFeeProxy)) {
// Funds are being sent back from EthereumFeeProxy
require(originalSender != address(0), 'No original sender stored');

// Forward the funds to the original sender
(bool forwardSuccess, ) = payable(originalSender).call{value: msg.value}('');
require(forwardSuccess, 'Forwarding to original sender failed');

// Clear the stored original sender
originalSender = address(0);
} else {
require(originalSender == address(0), 'Another request is in progress');

originalSender = msg.sender;

bytes memory data = abi.encodeWithSignature(
'transferWithReferenceAndFee(address,bytes,uint256,address)',
payable(payee),
paymentReference,
feeAmount,
Copy link
Contributor

Choose a reason for hiding this comment

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

In both ERC20SingleRequestProxy and EthereumSingleRequestProxy the fees amount are paid every time the receive method is called.
It won't be possible for a user to pay his requests with multiple payment as he'll need to pay for the fees several times. Is it the intended behavior ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is the intended behavior. The same thing happens when a request is created with feeAddress and feeAmount using the SDK; the fee is paid every time there is a payment.

This is a test request we did to see the behavior of the partial payments with fees , you can see the service fee being paid every time:
https://scan.request.network/request/0118bc428d569e48a62a64625bbd836dfc00c935caae8a201daf308438fce3ddd9

Copy link
Contributor

@leoslr leoslr Sep 25, 2024

Choose a reason for hiding this comment

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

The fees aren't paid necessarely every time there is a payment. For example if I have a request where I need to pay 100$ as well as 2$ of fees, I can:

  • Make on payment of 50$ and 2$ fees.
  • Another payment of 50$ and 0$ of fees

We'll detect both of these payments and the request will be considered paid, but with the current implementation of the contract you'd have to pay twice the 2$ of fees if you want to make partial payments.
Maybe the feeAmount should be updated after each payment ?

I don't understand the link you shared, the payment are not partial, each of them account for the full amount of the request (0.003 ETH)

Edit:
Using the SDK I might not be able to perform the two payments as I described them - though it's doable if I interact directly with the contract, and the request will be considered paid.
But I believe this is because the payment-processor package does not support well partial payment yet.
I don't think paying several times the fees in case of partial payment should be enforced at the contract level

payable(feeAddress)
);

(bool callSuccess, ) = address(ethereumFeeProxy).call{value: msg.value}(data);
require(callSuccess, 'Call to EthereumFeeProxy failed');
Comment on lines +77 to +78
Copy link

Choose a reason for hiding this comment

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

Handle potential failure of the external call gracefully.

Consider providing more detailed error handling or reverting state changes if the call to ethereumFeeProxy fails to enhance robustness.

Comment on lines +69 to +78
Copy link

Choose a reason for hiding this comment

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

Invoke transferWithReferenceAndFee via the interface instead of low-level call.

Using the interface method directly enhances code readability and safety by leveraging Solidity's function call mechanisms and type checking.

Apply this diff to update the function call:

         require(originalSender == address(0), 'Another request is in progress');

         originalSender = msg.sender;

-        bytes memory data = abi.encodeWithSignature(
-            'transferWithReferenceAndFee(address,bytes,uint256,address)',
-            payable(payee),
-            paymentReference,
-            feeAmount,
-            payable(feeAddress)
-        );
-
-        (bool callSuccess, ) = address(ethereumFeeProxy).call{value: msg.value}(data);
-        require(callSuccess, 'Call to EthereumFeeProxy failed');
+        ethereumFeeProxy.transferWithReferenceAndFee{value: msg.value}(
+            payable(payee),
+            paymentReference,
+            feeAmount,
+            payable(feeAddress)
+        );
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
bytes memory data = abi.encodeWithSignature(
'transferWithReferenceAndFee(address,bytes,uint256,address)',
payable(payee),
paymentReference,
feeAmount,
payable(feeAddress)
);
(bool callSuccess, ) = address(ethereumFeeProxy).call{value: msg.value}(data);
require(callSuccess, 'Call to EthereumFeeProxy failed');
originalSender = msg.sender;
ethereumFeeProxy.transferWithReferenceAndFee{value: msg.value}(
payable(payee),
paymentReference,
feeAmount,
payable(feeAddress)
);
require(callSuccess, 'Call to EthereumFeeProxy failed');

}
}
Comment on lines +53 to +80
Copy link

Choose a reason for hiding this comment

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

Ensure reentrancy protection covers all code paths.

By allowing calls from ethereumFeeProxy to bypass reentrancy protection, there might be unforeseen vulnerabilities. Reevaluate the necessity of this exception to prevent potential reentrancy attacks.

}
103 changes: 103 additions & 0 deletions packages/smart-contracts/src/contracts/SingleRequestProxyFactory.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.9;

import '@openzeppelin/contracts/access/Ownable.sol';
import './ERC20SingleRequestProxy.sol';
import './EthereumSingleRequestProxy.sol';

/**
* @title SingleRequestProxyFactory
* @notice This contract is used to create SingleRequestProxy instances
*/
contract SingleRequestProxyFactory is Ownable {
address public ethereumFeeProxy;
address public erc20FeeProxy;
Comment on lines +13 to +14
Copy link

Choose a reason for hiding this comment

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

Add NatSpec comments for public variables.

Consider adding NatSpec comments for the public variables ethereumFeeProxy and erc20FeeProxy to enhance code documentation and readability.


event EtheruemSingleRequestProxyCreated(
address indexed proxyAddress,
address indexed payee,
bytes indexed paymentReference
);
Comment on lines +16 to +20
Copy link

Choose a reason for hiding this comment

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

Fix the typo in the event name 'EtheruemSingleRequestProxyCreated'.

The event name EtheruemSingleRequestProxyCreated has a typo. It should be EthereumSingleRequestProxyCreated to correctly spell "Ethereum".

Apply this diff to correct the event name:

-event EtheruemSingleRequestProxyCreated(
+event EthereumSingleRequestProxyCreated(
    address indexed proxyAddress,
    address indexed payee,
    bytes indexed paymentReference
  );
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
event EtheruemSingleRequestProxyCreated(
address indexed proxyAddress,
address indexed payee,
bytes indexed paymentReference
);
event EthereumSingleRequestProxyCreated(
address indexed proxyAddress,
address indexed payee,
bytes indexed paymentReference
);


event ERC20SingleRequestProxyCreated(
address indexed proxyAddress,
address indexed payee,
address tokenAddress,
bytes indexed paymentReference
);

constructor(address _ethereumFeeProxy, address _erc20FeeProxy) Ownable() {
ethereumFeeProxy = _ethereumFeeProxy;
erc20FeeProxy = _erc20FeeProxy;
}
Comment on lines +29 to +32
Copy link

Choose a reason for hiding this comment

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

Add validation for fee proxy addresses in the constructor.

To prevent misconfiguration, add checks to ensure _ethereumFeeProxy and _erc20FeeProxy are not zero addresses when initializing the contract.

Add require statements in the constructor:

constructor(address _ethereumFeeProxy, address _erc20FeeProxy) Ownable() {
+   require(_ethereumFeeProxy != address(0), "ethereumFeeProxy address cannot be zero");
+   require(_erc20FeeProxy != address(0), "erc20FeeProxy address cannot be zero");
    ethereumFeeProxy = _ethereumFeeProxy;
    erc20FeeProxy = _erc20FeeProxy;
  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
constructor(address _ethereumFeeProxy, address _erc20FeeProxy) Ownable() {
ethereumFeeProxy = _ethereumFeeProxy;
erc20FeeProxy = _erc20FeeProxy;
}
constructor(address _ethereumFeeProxy, address _erc20FeeProxy) Ownable() {
require(_ethereumFeeProxy != address(0), "ethereumFeeProxy address cannot be zero");
require(_erc20FeeProxy != address(0), "erc20FeeProxy address cannot be zero");
ethereumFeeProxy = _ethereumFeeProxy;
erc20FeeProxy = _erc20FeeProxy;
}


/**
* @notice Creates a new EthereumSingleRequestProxy instance
* @param _payee The address of the payee
* @param _paymentReference The payment reference
* @param _feeAddress The address of the fee recipient
* @param _feeAmount The fee amount
* @return The address of the newly created proxy
*/
function createEthereumSingleRequestProxy(
address _payee,
bytes memory _paymentReference,
address _feeAddress,
uint256 _feeAmount
) external returns (address) {
EthereumSingleRequestProxy proxy = new EthereumSingleRequestProxy(
_payee,
_paymentReference,
ethereumFeeProxy,
_feeAddress,
_feeAmount
);
emit EtheruemSingleRequestProxyCreated(address(proxy), _payee, _paymentReference);
Copy link

Choose a reason for hiding this comment

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

Fix the typo in the event emission 'EtheruemSingleRequestProxyCreated'.

Update the event emission to use the corrected event name EthereumSingleRequestProxyCreated.

Apply this diff to correct the event emission:

-emit EtheruemSingleRequestProxyCreated(address(proxy), _payee, _paymentReference);
+emit EthereumSingleRequestProxyCreated(address(proxy), _payee, _paymentReference);
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
emit EtheruemSingleRequestProxyCreated(address(proxy), _payee, _paymentReference);
emit EthereumSingleRequestProxyCreated(address(proxy), _payee, _paymentReference);

return address(proxy);
}
Comment on lines +42 to +57
Copy link

Choose a reason for hiding this comment

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

Add input validation for function parameters in 'createEthereumSingleRequestProxy'.

It's recommended to validate critical input parameters to prevent errors or misuse. Add checks to ensure _payee, _feeAddress, and ethereumFeeProxy are not zero addresses.

Include require statements at the beginning of the function:

function createEthereumSingleRequestProxy(
    address _payee,
    bytes memory _paymentReference,
    address _feeAddress,
    uint256 _feeAmount
  ) external returns (address) {
+   require(_payee != address(0), "Payee address cannot be zero");
+   require(_feeAddress != address(0), "Fee address cannot be zero");
+   require(ethereumFeeProxy != address(0), "ethereumFeeProxy address not set");
    EthereumSingleRequestProxy proxy = new EthereumSingleRequestProxy(
      _payee,
      _paymentReference,
      ethereumFeeProxy,
      _feeAddress,
      _feeAmount
    );
    emit EthereumSingleRequestProxyCreated(address(proxy), _payee, _paymentReference);
    return address(proxy);
  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function createEthereumSingleRequestProxy(
address _payee,
bytes memory _paymentReference,
address _feeAddress,
uint256 _feeAmount
) external returns (address) {
EthereumSingleRequestProxy proxy = new EthereumSingleRequestProxy(
_payee,
_paymentReference,
ethereumFeeProxy,
_feeAddress,
_feeAmount
);
emit EtheruemSingleRequestProxyCreated(address(proxy), _payee, _paymentReference);
return address(proxy);
}
function createEthereumSingleRequestProxy(
address _payee,
bytes memory _paymentReference,
address _feeAddress,
uint256 _feeAmount
) external returns (address) {
require(_payee != address(0), "Payee address cannot be zero");
require(_feeAddress != address(0), "Fee address cannot be zero");
require(ethereumFeeProxy != address(0), "ethereumFeeProxy address not set");
EthereumSingleRequestProxy proxy = new EthereumSingleRequestProxy(
_payee,
_paymentReference,
ethereumFeeProxy,
_feeAddress,
_feeAmount
);
emit EthereumSingleRequestProxyCreated(address(proxy), _payee, _paymentReference);
return address(proxy);
}


/**
* @notice Creates a new ERC20SingleRequestProxy instance
* @param _payee The address of the payee
* @param _tokenAddress The address of the token
* @param _paymentReference The payment reference
* @param _feeAddress The address of the fee recipient
* @param _feeAmount The fee amount
* @return The address of the newly created proxy
*/
function createERC20SingleRequestProxy(
address _payee,
address _tokenAddress,
bytes memory _paymentReference,
address _feeAddress,
uint256 _feeAmount
) external returns (address) {
ERC20SingleRequestProxy proxy = new ERC20SingleRequestProxy(
_payee,
_tokenAddress,
_feeAddress,
_feeAmount,
_paymentReference,
erc20FeeProxy
);

emit ERC20SingleRequestProxyCreated(address(proxy), _payee, _tokenAddress, _paymentReference);
return address(proxy);
}
Comment on lines +68 to +86
Copy link

Choose a reason for hiding this comment

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

Add input validation for function parameters in 'createERC20SingleRequestProxy'.

To ensure robustness, add validations for critical input parameters. Ensure _payee, _tokenAddress, _feeAddress, and erc20FeeProxy are not zero addresses.

Include require statements at the start of the function:

function createERC20SingleRequestProxy(
    address _payee,
    address _tokenAddress,
    bytes memory _paymentReference,
    address _feeAddress,
    uint256 _feeAmount
  ) external returns (address) {
+   require(_payee != address(0), "Payee address cannot be zero");
+   require(_tokenAddress != address(0), "Token address cannot be zero");
+   require(_feeAddress != address(0), "Fee address cannot be zero");
+   require(erc20FeeProxy != address(0), "erc20FeeProxy address not set");
    ERC20SingleRequestProxy proxy = new ERC20SingleRequestProxy(
      _payee,
      _tokenAddress,
      _feeAddress,
      _feeAmount,
      _paymentReference,
      erc20FeeProxy
    );
    emit ERC20SingleRequestProxyCreated(address(proxy), _payee, _tokenAddress, _paymentReference);
    return address(proxy);
  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function createERC20SingleRequestProxy(
address _payee,
address _tokenAddress,
bytes memory _paymentReference,
address _feeAddress,
uint256 _feeAmount
) external returns (address) {
ERC20SingleRequestProxy proxy = new ERC20SingleRequestProxy(
_payee,
_tokenAddress,
_feeAddress,
_feeAmount,
_paymentReference,
erc20FeeProxy
);
emit ERC20SingleRequestProxyCreated(address(proxy), _payee, _tokenAddress, _paymentReference);
return address(proxy);
}
function createERC20SingleRequestProxy(
address _payee,
address _tokenAddress,
bytes memory _paymentReference,
address _feeAddress,
uint256 _feeAmount
) external returns (address) {
require(_payee != address(0), "Payee address cannot be zero");
require(_tokenAddress != address(0), "Token address cannot be zero");
require(_feeAddress != address(0), "Fee address cannot be zero");
require(erc20FeeProxy != address(0), "erc20FeeProxy address not set");
ERC20SingleRequestProxy proxy = new ERC20SingleRequestProxy(
_payee,
_tokenAddress,
_feeAddress,
_feeAmount,
_paymentReference,
erc20FeeProxy
);
emit ERC20SingleRequestProxyCreated(address(proxy), _payee, _tokenAddress, _paymentReference);
return address(proxy);
}


/**
* @notice Updates the ERC20FeeProxy address
* @param _newERC20FeeProxy The new ERC20FeeProxy address
*/
function setERC20FeeProxy(address _newERC20FeeProxy) external onlyOwner {
erc20FeeProxy = _newERC20FeeProxy;
}
Comment on lines +92 to +94
Copy link

Choose a reason for hiding this comment

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

Add validation for new ERC20FeeProxy address in 'setERC20FeeProxy'.

To prevent setting an invalid address, add a check to ensure _newERC20FeeProxy is not the zero address before updating erc20FeeProxy.

Implement the validation:

function setERC20FeeProxy(address _newERC20FeeProxy) external onlyOwner {
+   require(_newERC20FeeProxy != address(0), "New ERC20FeeProxy address cannot be zero");
    erc20FeeProxy = _newERC20FeeProxy;
  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function setERC20FeeProxy(address _newERC20FeeProxy) external onlyOwner {
erc20FeeProxy = _newERC20FeeProxy;
}
function setERC20FeeProxy(address _newERC20FeeProxy) external onlyOwner {
require(_newERC20FeeProxy != address(0), "New ERC20FeeProxy address cannot be zero");
erc20FeeProxy = _newERC20FeeProxy;
}

Consider emitting an event when updating 'erc20FeeProxy'.

To enhance transparency and enable off-chain monitoring, emit an event when the erc20FeeProxy address is updated.

Define an event and emit it:

+ event ERC20FeeProxyUpdated(address indexed oldAddress, address indexed newAddress);

function setERC20FeeProxy(address _newERC20FeeProxy) external onlyOwner {
  require(_newERC20FeeProxy != address(0), "New ERC20FeeProxy address cannot be zero");
+ emit ERC20FeeProxyUpdated(erc20FeeProxy, _newERC20FeeProxy);
  erc20FeeProxy = _newERC20FeeProxy;
}

Committable suggestion was skipped due to low confidence.


/**
* @notice Updates the EthereumFeeProxy address
* @param _newEthereumFeeProxy The new EthereumFeeProxy address
*/
function setEthereumFeeProxy(address _newEthereumFeeProxy) external onlyOwner {
ethereumFeeProxy = _newEthereumFeeProxy;
}
Comment on lines +100 to +102
Copy link

Choose a reason for hiding this comment

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

Add validation for new EthereumFeeProxy address in 'setEthereumFeeProxy'.

Ensure that _newEthereumFeeProxy is not the zero address before updating ethereumFeeProxy to avoid misconfiguration.

Implement the validation:

function setEthereumFeeProxy(address _newEthereumFeeProxy) external onlyOwner {
+   require(_newEthereumFeeProxy != address(0), "New EthereumFeeProxy address cannot be zero");
    ethereumFeeProxy = _newEthereumFeeProxy;
  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function setEthereumFeeProxy(address _newEthereumFeeProxy) external onlyOwner {
ethereumFeeProxy = _newEthereumFeeProxy;
}
function setEthereumFeeProxy(address _newEthereumFeeProxy) external onlyOwner {
require(_newEthereumFeeProxy != address(0), "New EthereumFeeProxy address cannot be zero");
ethereumFeeProxy = _newEthereumFeeProxy;
}

Consider emitting an event when updating 'ethereumFeeProxy'.

To improve transparency and facilitate monitoring, emit an event when the ethereumFeeProxy address is updated.

Define an event and emit it:

+ event EthereumFeeProxyUpdated(address indexed oldAddress, address indexed newAddress);

function setEthereumFeeProxy(address _newEthereumFeeProxy) external onlyOwner {
  require(_newEthereumFeeProxy != address(0), "New EthereumFeeProxy address cannot be zero");
+ emit EthereumFeeProxyUpdated(ethereumFeeProxy, _newEthereumFeeProxy);
  ethereumFeeProxy = _newEthereumFeeProxy;
}

Committable suggestion was skipped due to low confidence.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract MockEthereumFeeProxy {
function transferWithReferenceAndFee(
address payable _to,
bytes calldata _paymentReference,
uint256 _feeAmount,
address payable _feeAddress
) external payable {
// Do nothing, just accept the funds
}
Comment on lines +1 to +12
Copy link

Choose a reason for hiding this comment

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

LGTM! Consider adding an event emission for better testability.

The MockEthereumFeeProxy contract and the transferWithReferenceAndFee function are correctly implemented for a mock contract. The function appropriately accepts parameters without using them, which is suitable for testing scenarios.

To enhance testability, consider emitting an event in the transferWithReferenceAndFee function. This would allow test cases to verify that the function was called with the correct parameters. Here's a suggested implementation:

+ event TransferWithReferenceAndFeeCalled(address to, bytes paymentReference, uint256 feeAmount, address feeAddress, uint256 value);

  function transferWithReferenceAndFee(
    address payable _to,
    bytes calldata _paymentReference,
    uint256 _feeAmount,
    address payable _feeAddress
  ) external payable {
    // Do nothing, just accept the funds
+   emit TransferWithReferenceAndFeeCalled(_to, _paymentReference, _feeAmount, _feeAddress, msg.value);
  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
contract MockEthereumFeeProxy {
function transferWithReferenceAndFee(
address payable _to,
bytes calldata _paymentReference,
uint256 _feeAmount,
address payable _feeAddress
) external payable {
// Do nothing, just accept the funds
}
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
contract MockEthereumFeeProxy {
event TransferWithReferenceAndFeeCalled(address to, bytes paymentReference, uint256 feeAmount, address feeAddress, uint256 value);
function transferWithReferenceAndFee(
address payable _to,
bytes calldata _paymentReference,
uint256 _feeAmount,
address payable _feeAddress
) external payable {
// Do nothing, just accept the funds
emit TransferWithReferenceAndFeeCalled(_to, _paymentReference, _feeAmount, _feeAddress, msg.value);
}


function sendFundsBack(address payable _to, uint256 _amount) external {
(bool success, ) = _to.call{value: _amount}('');
require(success, 'Failed to send funds back');
}
Comment on lines +14 to +17
Copy link

Choose a reason for hiding this comment

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

LGTM! Consider implementing checks-effects-interactions pattern.

The sendFundsBack function correctly uses a low-level call to transfer Ether and includes a require statement to ensure the transfer's success. This is a good practice for handling Ether transfers.

To enhance security and prevent potential reentrancy attacks, consider implementing the checks-effects-interactions pattern. Here's a suggested implementation:

+ uint256 private _balance;

+ function deposit() external payable {
+     _balance += msg.value;
+ }

  function sendFundsBack(address payable _to, uint256 _amount) external {
+     require(_balance >= _amount, "Insufficient balance");
+     _balance -= _amount;
      (bool success, ) = _to.call{value: _amount}('');
      require(success, 'Failed to send funds back');
  }

This implementation separates the state changes from the external call, reducing the risk of reentrancy attacks.

Committable suggestion was skipped due to low confidence.


receive() external payable {}
Copy link

Choose a reason for hiding this comment

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

LGTM! Consider adding an event emission for better testability and transparency.

The receive function is correctly implemented to allow the contract to accept Ether transfers without calling a specific function. This is appropriate for a mock contract.

To enhance testability and transparency, consider emitting an event when Ether is received. Here's a suggested implementation:

+ event EtherReceived(address sender, uint256 amount);

  receive() external payable {
+     emit EtherReceived(msg.sender, msg.value);
  }

This addition would make it easier to track and verify Ether transfers to the contract during testing.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
receive() external payable {}
event EtherReceived(address sender, uint256 amount);
receive() external payable {
emit EtherReceived(msg.sender, msg.value);
}

}
15 changes: 15 additions & 0 deletions packages/smart-contracts/src/contracts/test/TestToken.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// SPDX-License-Identifier: MIT
// Compatible with OpenZeppelin Contracts ^5.0.0
pragma solidity ^0.8.9;

import '@openzeppelin/contracts/token/ERC20/ERC20.sol';
import '@openzeppelin/contracts/access/Ownable.sol';
import '@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol';

contract TestToken is ERC20, Ownable, ERC20Permit {
constructor(address initialOwner) ERC20('TestToken', 'TTK') Ownable() ERC20Permit('TestToken') {}
Copy link

Choose a reason for hiding this comment

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

Constructor looks good, but consider parameterizing token name and symbol.

The constructor correctly initializes all inherited contracts and allows for flexible ownership assignment. However, hardcoding the token name and symbol might limit the contract's reusability.

Consider parameterizing the token name and symbol:

-constructor(address initialOwner) ERC20('TestToken', 'TTK') Ownable() ERC20Permit('TestToken') {}
+constructor(
+    string memory name,
+    string memory symbol,
+    address initialOwner
+) ERC20(name, symbol) Ownable() ERC20Permit(name) {}

This change would make the contract more flexible for different use cases.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
constructor(address initialOwner) ERC20('TestToken', 'TTK') Ownable() ERC20Permit('TestToken') {}
constructor(
string memory name,
string memory symbol,
address initialOwner
) ERC20(name, symbol) Ownable() ERC20Permit(name) {}


function mint(address to, uint256 amount) public onlyOwner {
_mint(to, amount);
}
Comment on lines +12 to +14
Copy link

Choose a reason for hiding this comment

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

Mint function is correct, but consider adding an event emission.

The mint function is correctly implemented with the onlyOwner modifier for access control. It uses the internal _mint function as expected.

Consider adding an event emission after minting for better off-chain tracking:

function mint(address to, uint256 amount) public onlyOwner {
    _mint(to, amount);
+   emit TokensMinted(to, amount);
}

+event TokensMinted(address indexed to, uint256 amount);

This addition would improve the contract's observability and make it easier for external systems to track minting operations.

Committable suggestion was skipped due to low confidence.

}
Loading