Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(contracts-rfq): relay/prove/claim with different address [SLT-130] #3138
feat(contracts-rfq): relay/prove/claim with different address [SLT-130] #3138
Changes from 16 commits
e85881b
b4b7783
1a521d6
0fd674b
9e8a628
1d28940
b3a0eb9
c4d2235
e368351
7a5c181
8d38754
9f42c69
2cff786
0f99c3e
0f39b57
3031dc8
4b72978
69aa870
a98353d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning
Code scanning / Slither
Different pragma directives are used Warning
Check warning
Code scanning / Slither
Different pragma directives are used Warning
Check warning
Code scanning / Slither
Incorrect versions of Solidity Warning
Check warning
Code scanning / Slither
Incorrect versions of Solidity Warning
Check warning
Code scanning / Slither
Incorrect versions of Solidity Warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address Reentrancy in
_pullToken
FunctionThe
_pullToken
function performs external calls that could introduce reentrancy risks, especially when transferring ETH.Recommendation:
_pullToken
are protected withnonReentrant
.Check warning
Code scanning / Slither
Uninitialized local variables Medium
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declare Missing Variables and Constants Used in Calculations
The variables
protocolFeeRate
,FEE_BPS
,protocolFees
, andchainGasAmount
are used but not declared in the contract. This will result in compilation errors.Please declare these variables and constants. For example:
Also applies to: 150-151, 240-240
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Reentrancy Protection to
bridge
FunctionThe
bridge
function is an external payable function that involves token transfers via_pullToken
, which may invoke external code. Since state variables are updated after external calls, there's a risk of reentrancy attacks.Recommendation:
Introduce reentrancy protection by inheriting from OpenZeppelin's
ReentrancyGuard
and adding thenonReentrant
modifier to thebridge
function.Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
Check notice
Code scanning / Slither
Block timestamp Low
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restrict
relayer
Parameter inrelay
FunctionThe
relay
function allows callers to specify anyrelayer
address, which could lead to misrepresentation or misuse.Recommendation:
Restrict the
relayer
tomsg.sender
to ensure the correct relayer is recorded.Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Reentrancy Guard to the
relay
FunctionThe
relay
function ispublic
andpayable
, and it calls_pullToken
, which involves external calls and token transfers. This could expose the contract to reentrancy attacks if not protected.Apply the following changes to secure the
relay
function:Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
Check notice
Code scanning / Slither
Block timestamp Low
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check for Duplicate Proofs in
prove
FunctionThere is no check to prevent a proof from being submitted multiple times for the same transaction.
Recommendation:
Add a check to ensure that a proof cannot be submitted if one already exists.
function prove(bytes memory request, bytes32 destTxHash) public onlyRole(RELAYER_ROLE) { bytes32 transactionId = keccak256(request); + if (bridgeStatuses[transactionId] != BridgeStatus.REQUESTED) revert StatusIncorrect(); // existing code }
Validate
relayer
Address inprove
FunctionIn the
prove
function, therelayer
address is provided as a parameter without validation, which could allow incorrect association of the relayer.Recommendation:
Use
msg.sender
as the relayer to ensure accuracy and security.Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure Safe Arithmetic in
_timeSince
FunctionThe
_timeSince
function uses unchecked subtraction betweenuint96
timestamps, which could lead to underflow ifproof.timestamp
is greater thanblock.timestamp
.Recommendation:
Use safe arithmetic or validate timestamps to prevent underflow.
Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid Reverting in
canClaim
FunctionThe
canClaim
function reverts on incorrect status or sender, which may not be ideal for a view function meant to inform callers.Recommendation:
Return
false
instead of reverting to provide a smoother user experience.Committable suggestion
Check notice
Code scanning / Slither
Block timestamp Low
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use SafeERC20 Library for Token Transfers
When transferring tokens, it's safer to use OpenZeppelin's
SafeERC20
library to handle any potential anomalies with ERC20 tokens that do not return a boolean value on transfer.Modify the token transfer to use
safeTransfer
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplify Recipient Logic in
claim
FunctionThe recipient logic in the
claim
function adds complexity and could be streamlined.Recommendation:
Standardize the recipient to be the
proof.relayer
to reduce conditional checks.Committable suggestion
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
Check notice
Code scanning / Slither
Block timestamp Low
Check notice
Code scanning / Slither
Block timestamp Low
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use SafeERC20 Library in
refund
FunctionSimilar to the previous suggestion, ensure the use of
safeTransfer
when refunding tokens to the user to handle any potential token transfer issues.Update the token transfer in the
refund
function:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Reentrancy Protection to
refund
FunctionThe
refund
function involves transferring tokens back to the sender after external calls, which may lead to reentrancy vulnerabilities.Recommendation:
Apply the
nonReentrant
modifier to therefund
function.Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Reentrancy Guard to the
refund
FunctionThe
refund
function isexternal
and transfers tokens back to the user usingtoken.universalTransfer(to, amount);
. Without reentrancy protection, this could be exploited by malicious actors.Apply the following change to add a reentrancy guard:
Committable suggestion
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
Check notice
Code scanning / Slither
Block timestamp Low
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update Solidity pragma version for broader compatibility
Consider using a caret (
^
) pragma to allow for future compatible compiler versions. Changingpragma solidity ^0.8.20;
topragma solidity ^0.8.0;
can increase the range of compiler versions that can be used, providing more flexibility for developers.Apply this change:
Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate the necessity of the
relayer
parameter inrelay
functionThe
relay
function includes arelayer
address parameter. Since interfaces cannot implement logic, it's important to ensure that the implementation will correctly handle this parameter. Consider whether therelayer
address should be obtained frommsg.sender
within the implementation to prevent potential misuse where a caller could specify a differentrelayer
address, which might introduce security risks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review the necessity of passing
relayer
as a parameterPassing the
relayer
address explicitly in therelay
function may introduce security risks. Since any caller could specify an arbitraryrelayer
address, consider usingmsg.sender
as the relayer to ensure that the entity initiating the relay is correctly associated.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assess the use of the
relayer
parameter inprove
functionSimilar to the
relay
function, theprove
function takes arelayer
address as a parameter. Verify whether it's necessary to pass therelayer
explicitly, or if it should be derived frommsg.sender
during implementation. This could prevent scenarios where a malicious actor proves a transaction on behalf of another relayer without authorization.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate the
relayer
parameter inprove
functionSimilar to the
relay
function, passingrelayer
as a parameter inprove
could allow malicious actors to misrepresent the relayer's identity. Ensure that the implementation validates the authenticity of therelayer
address or considers usingmsg.sender
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enhance function documentation for clarity
The function comments can be improved to provide clearer guidance:
relayer
.Consider revising the documentation as follows:
For
relay
:Apply similar enhancements to the
prove
andclaim
function comments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add events for important state-changing functions
Interfaces often include event declarations for significant actions to enhance transparency and enable off-chain monitoring. Consider adding events for the
relay
,prove
, andclaim
functions to allow tracking when these actions occur in the implementing contracts.Apply the following changes to declare events:
Committable suggestion