-
Notifications
You must be signed in to change notification settings - Fork 31
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): arbitrary calls without additional native value [SLT-233] #3215
Changes from 13 commits
7dba739
6b35618
6dc842c
c746727
543747b
3200f09
df0a3ab
5a2205e
30ffab5
48fad07
4ce70d9
ad70449
693b432
2a785ca
0a222fe
02ba663
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,13 +2,15 @@ | |
pragma solidity 0.8.24; | ||
|
||
import {SafeERC20, IERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; | ||
import {Address} from "@openzeppelin/contracts/utils/Address.sol"; | ||
|
||
import {UniversalTokenLib} from "./libs/UniversalToken.sol"; | ||
|
||
import {Admin} from "./Admin.sol"; | ||
import {IFastBridge} from "./interfaces/IFastBridge.sol"; | ||
import {IFastBridgeV2} from "./interfaces/IFastBridgeV2.sol"; | ||
import {IFastBridgeV2Errors} from "./interfaces/IFastBridgeV2Errors.sol"; | ||
import {IFastBridgeRecipient} from "./interfaces/IFastBridgeRecipient.sol"; | ||
|
||
/// @notice FastBridgeV2 is a contract for bridging tokens across chains. | ||
contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { | ||
|
@@ -24,6 +26,9 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { | |
/// @notice Minimum deadline period to relay a requested bridge transaction | ||
uint256 public constant MIN_DEADLINE_PERIOD = 30 minutes; | ||
|
||
/// @notice Maximum length of accepted callParams | ||
uint256 public constant MAX_CALL_PARAMS_LENGTH = 2 ** 16 - 1; | ||
|
||
/// @notice Status of the bridge tx on origin chain | ||
mapping(bytes32 => BridgeTxDetails) public bridgeTxDetails; | ||
/// @notice Relay details on destination chain | ||
|
@@ -45,7 +50,12 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { | |
function bridge(BridgeParams memory params) external payable { | ||
bridge({ | ||
params: params, | ||
paramsV2: BridgeParamsV2({quoteRelayer: address(0), quoteExclusivitySeconds: 0, quoteId: bytes("")}) | ||
paramsV2: BridgeParamsV2({ | ||
quoteRelayer: address(0), | ||
quoteExclusivitySeconds: 0, | ||
quoteId: bytes(""), | ||
callParams: bytes("") | ||
}) | ||
}); | ||
} | ||
|
||
|
@@ -132,6 +142,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { | |
if (params.sender == address(0) || params.to == address(0)) revert ZeroAddress(); | ||
if (params.originToken == address(0) || params.destToken == address(0)) revert ZeroAddress(); | ||
if (params.deadline < block.timestamp + MIN_DEADLINE_PERIOD) revert DeadlineTooShort(); | ||
if (paramsV2.callParams.length > MAX_CALL_PARAMS_LENGTH) revert CallParamsLengthAboveMax(); | ||
int256 exclusivityEndTime = int256(block.timestamp) + paramsV2.quoteExclusivitySeconds; | ||
// exclusivityEndTime must be in range (0 .. params.deadline] | ||
if (exclusivityEndTime <= 0 || exclusivityEndTime > int256(params.deadline)) { | ||
|
@@ -163,7 +174,8 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { | |
nonce: senderNonces[params.sender]++, // increment nonce on every bridge | ||
exclusivityRelayer: paramsV2.quoteRelayer, | ||
// We checked exclusivityEndTime to be in range (0 .. params.deadline] above, so can safely cast | ||
exclusivityEndTime: uint256(exclusivityEndTime) | ||
exclusivityEndTime: uint256(exclusivityEndTime), | ||
callParams: paramsV2.callParams | ||
}) | ||
); | ||
bytes32 transactionId = keccak256(request); | ||
|
@@ -214,18 +226,29 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { | |
address token = transaction.destToken; | ||
uint256 amount = transaction.destAmount; | ||
|
||
uint256 rebate = chainGasAmount; | ||
if (!transaction.sendChainGas) { | ||
// forward erc20 | ||
rebate = 0; | ||
if (transaction.callParams.length == 0) { | ||
// No arbitrary call requested, so we just transfer the tokens | ||
_pullToken(to, token, amount); | ||
} else if (token == UniversalTokenLib.ETH_ADDRESS) { | ||
// lump in gas rebate into amount in native gas token | ||
_pullToken(to, token, amount + rebate); | ||
} else { | ||
// forward erc20 then forward gas rebate in native gas token | ||
} else if (token != UniversalTokenLib.ETH_ADDRESS) { | ||
ChiTimesChi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Arbitrary call requested with ERC20: transfer the tokens first | ||
_pullToken(to, token, amount); | ||
_pullToken(to, UniversalTokenLib.ETH_ADDRESS, rebate); | ||
// Follow up with the hook function call | ||
ChiTimesChi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_checkedCallRecipient({ | ||
recipient: to, | ||
msgValue: 0, | ||
token: token, | ||
amount: amount, | ||
callParams: transaction.callParams | ||
}); | ||
} else { | ||
// Arbitrary call requested with ETH: combine the ETH transfer with the call | ||
_checkedCallRecipient({ | ||
recipient: to, | ||
msgValue: amount, | ||
token: token, | ||
amount: amount, | ||
callParams: transaction.callParams | ||
}); | ||
} | ||
|
||
emit BridgeRelayed( | ||
|
@@ -237,7 +260,8 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { | |
transaction.destToken, | ||
transaction.originAmount, | ||
transaction.destAmount, | ||
rebate | ||
// chainGasAmount is 0 since the gas rebate function is deprecated | ||
0 | ||
); | ||
} | ||
|
||
|
@@ -327,6 +351,31 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors { | |
} | ||
} | ||
|
||
/// @notice Calls the Recipient's hook function with the specified callParams and performs | ||
/// all the necessary checks for the returned value. | ||
function _checkedCallRecipient( | ||
address recipient, | ||
uint256 msgValue, | ||
address token, | ||
uint256 amount, | ||
bytes memory callParams | ||
) | ||
internal | ||
{ | ||
bytes memory hookData = | ||
abi.encodeCall(IFastBridgeRecipient.fastBridgeTransferReceived, (token, amount, callParams)); | ||
// This will bubble any revert messages from the hook function | ||
bytes memory returnData = Address.functionCallWithValue({target: recipient, data: hookData, value: msgValue}); | ||
// Explicit revert if no return data at all | ||
if (returnData.length == 0) revert RecipientNoReturnValue(); | ||
// Check that exactly a single return value was returned | ||
if (returnData.length != 32) revert RecipientIncorrectReturnValue(); | ||
// Return value should be abi-encoded hook function selector | ||
if (bytes32(returnData) != bytes32(IFastBridgeRecipient.fastBridgeTransferReceived.selector)) { | ||
revert RecipientIncorrectReturnValue(); | ||
} | ||
} | ||
Comment on lines
+360
to
+383
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider adding reentrancy protection to external calls The Consider adding the Apply this diff to add reentrancy protection: +import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
-contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors {
+contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors, ReentrancyGuard {
...
function relay(bytes memory request, address relayer) public payable
+ nonReentrant
{
...
}
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also a bit surprised this isn't non-reenrant. Can you explain why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @trajan0x, the Here's the relevant part of the code: // Mark bridge transaction as relayed
bridgeRelayDetails[transactionId] = BridgeRelay({
blockNumber: uint48(block.number),
blockTimestamp: uint48(block.timestamp),
relayer: relayer
});
// All state changes have been done at this point, can proceed to the external calls.
// This follows the checks-effects-interactions pattern to mitigate potential reentrancy attacks.
if (transaction.callParams.length == 0) {
// No arbitrary call requested, so we just pull the tokens from the Relayer to the recipient,
// or transfer ETH to the recipient (if token is ETH_ADDRESS)
_pullToken(to, token, amount);
} else if (token != UniversalTokenLib.ETH_ADDRESS) {
// Arbitrary call requested with ERC20: pull the tokens from the Relayer to the recipient first
_pullToken(to, token, amount);
// Follow up with the hook function call
_checkedCallRecipient({
recipient: to,
msgValue: 0,
token: token,
amount: amount,
callParams: transaction.callParams
});
} else {
// Arbitrary call requested with ETH: combine the ETH transfer with the call
_checkedCallRecipient({
recipient: to,
msgValue: amount,
token: token,
amount: amount,
callParams: transaction.callParams
});
} By updating the However, adding the To add reentrancy protection, you can inherit from OpenZeppelin's import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors, ReentrancyGuard {
...
function relay(bytes memory request, address relayer) public payable
nonReentrant
{
...
}
} Implementing this change would enhance the contract's security by enforcing non-reentrancy at the function level, ensuring the contract remains robust against potential reentrancy attacks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @trajan0x this PR doesn't add any additional risk vectors: legacy ETH transfers are also done through the recipient call. Therefore no additional reenrancy protection was introduced here. I suggest to leave it out of this PR as being a potentially more of a global thing we might or might not want to deal with. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
/// @notice Calculates time since proof submitted | ||
/// @dev proof.timestamp stores casted uint40(block.timestamp) block timestamps for gas optimization | ||
/// _timeSince(proof) can accomodate rollover case when block.timestamp > type(uint40).max but | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity ^0.8.0; | ||
|
||
|
||
interface IFastBridgeRecipient { | ||
function fastBridgeTransferReceived( | ||
address token, | ||
uint256 amount, | ||
bytes memory callParams | ||
) | ||
external | ||
payable | ||
returns (bytes4); | ||
} |
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.
LGTM with suggestion: New checks added for callParams and exclusivity period.
The addition of checks for callParams length and exclusivity period is good. However, there's a potential issue with the exclusivityEndTime calculation.
The casting of
block.timestamp
toint256
could cause overflow issues in the far future. Consider usinguint256
instead:This change eliminates the need for the
<= 0
check and prevents potential overflow issues.📝 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.
Solidity ^0.8 prevents overflows by trigering hard revert.
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.
@trajan0x, you're correct. Since Solidity 0.8, integer overflows and underflows cause a revert, so the explicit checks may not be necessary. Thank you for the clarification.
(・・?)
✏️ Learnings added