-
Notifications
You must be signed in to change notification settings - Fork 30
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
solhint warning fixes [SLT-287] #3207
Changes from 1 commit
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 |
---|---|---|
|
@@ -34,46 +34,10 @@ | |
// @dev the block the contract was deployed at | ||
uint256 public immutable deployBlock; | ||
|
||
function bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status) { | ||
return bridgeTxDetails[transactionId].status; | ||
} | ||
|
||
function bridgeProofs(bytes32 transactionId) public view returns (uint96 timestamp, address relayer) { | ||
timestamp = bridgeTxDetails[transactionId].proofBlockTimestamp; | ||
relayer = bridgeTxDetails[transactionId].proofRelayer; | ||
} | ||
|
||
constructor(address _owner) Admin(_owner) { | ||
deployBlock = block.number; | ||
} | ||
|
||
/// @notice Pulls a requested token from the user to the requested recipient. | ||
/// @dev Be careful of re-entrancy issues when msg.value > 0 and recipient != address(this) | ||
function _pullToken(address recipient, address token, uint256 amount) internal returns (uint256 amountPulled) { | ||
if (token != UniversalTokenLib.ETH_ADDRESS) { | ||
token.assertIsContract(); | ||
// Record token balance before transfer | ||
amountPulled = IERC20(token).balanceOf(recipient); | ||
// Token needs to be pulled only if msg.value is zero | ||
// This way user can specify WETH as the origin asset | ||
IERC20(token).safeTransferFrom(msg.sender, recipient, amount); | ||
// Use the difference between the recorded balance and the current balance as the amountPulled | ||
amountPulled = IERC20(token).balanceOf(recipient) - amountPulled; | ||
} else { | ||
// Otherwise, we need to check that ETH amount matches msg.value | ||
if (amount != msg.value) revert MsgValueIncorrect(); | ||
// Transfer value to recipient if not this address | ||
if (recipient != address(this)) token.universalTransfer(recipient, amount); | ||
// We will forward msg.value in the external call later, if recipient is not this contract | ||
amountPulled = msg.value; | ||
} | ||
} | ||
|
||
/// @inheritdoc IFastBridge | ||
function getBridgeTransaction(bytes memory request) public pure returns (BridgeTransaction memory) { | ||
return abi.decode(request, (BridgeTransaction)); | ||
} | ||
|
||
/// @inheritdoc IFastBridge | ||
function bridge(BridgeParams memory params) external payable { | ||
// check bridge params | ||
|
@@ -130,6 +94,68 @@ | |
relay(request, msg.sender); | ||
} | ||
|
||
/// @inheritdoc IFastBridge | ||
function prove(bytes memory request, bytes32 destTxHash) external { | ||
bytes32 transactionId = keccak256(request); | ||
prove(transactionId, destTxHash, msg.sender); | ||
} | ||
|
||
/// @inheritdoc IFastBridgeV2 | ||
function claim(bytes memory request) external { | ||
claim(request, address(0)); | ||
} | ||
|
||
/// @inheritdoc IFastBridge | ||
function dispute(bytes32 transactionId) external onlyRole(GUARD_ROLE) { | ||
if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); | ||
if (_timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) > DISPUTE_PERIOD) { | ||
revert DisputePeriodPassed(); | ||
} | ||
|
||
// @dev relayer gets slashed effectively if dest relay has gone thru | ||
bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED; | ||
bridgeTxDetails[transactionId].proofRelayer = address(0); | ||
bridgeTxDetails[transactionId].proofBlockTimestamp = 0; | ||
bridgeTxDetails[transactionId].proofBlockNumber = 0; | ||
|
||
emit BridgeProofDisputed(transactionId, msg.sender); | ||
} | ||
Comment on lines
+109
to
+122
Check notice Code scanning / Slither Block timestamp Low
FastBridgeV2.dispute(bytes32) uses timestamp for comparisons
Dangerous comparisons: - _timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) > DISPUTE_PERIOD |
||
|
||
/// @inheritdoc IFastBridge | ||
function refund(bytes memory request) external { | ||
bytes32 transactionId = keccak256(request); | ||
|
||
BridgeTransaction memory transaction = getBridgeTransaction(request); | ||
|
||
if (bridgeTxDetails[transactionId].status != BridgeStatus.REQUESTED) revert StatusIncorrect(); | ||
|
||
if (hasRole(REFUNDER_ROLE, msg.sender)) { | ||
// Refunder can refund if deadline has passed | ||
if (block.timestamp <= transaction.deadline) revert DeadlineNotExceeded(); | ||
} else { | ||
// Permissionless refund is allowed after REFUND_DELAY | ||
if (block.timestamp <= transaction.deadline + REFUND_DELAY) revert DeadlineNotExceeded(); | ||
} | ||
|
||
// if all checks passed, set to REFUNDED status | ||
bridgeTxDetails[transactionId].status = BridgeStatus.REFUNDED; | ||
|
||
// transfer origin collateral back to original sender | ||
address to = transaction.originSender; | ||
address token = transaction.originToken; | ||
uint256 amount = transaction.originAmount + transaction.originFeeAmount; | ||
token.universalTransfer(to, amount); | ||
|
||
emit BridgeDepositRefunded(transactionId, to, token, amount); | ||
} | ||
Comment on lines
+125
to
+150
Check notice Code scanning / Slither Reentrancy vulnerabilities Low
Reentrancy in FastBridgeV2.refund(bytes):
External calls: - token.universalTransfer(to,amount) Event emitted after the call(s): - BridgeDepositRefunded(transactionId,to,token,amount)
Comment on lines
+125
to
+150
Check notice Code scanning / Slither Block timestamp Low
FastBridgeV2.refund(bytes) uses timestamp for comparisons
Dangerous comparisons: - block.timestamp <= transaction.deadline - block.timestamp <= transaction.deadline + REFUND_DELAY |
||
|
||
/// @inheritdoc IFastBridge | ||
function canClaim(bytes32 transactionId, address relayer) external view returns (bool) { | ||
if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); | ||
if (bridgeTxDetails[transactionId].proofRelayer != relayer) revert SenderIncorrect(); | ||
return _timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) > DISPUTE_PERIOD; | ||
} | ||
Check notice Code scanning / Slither Block timestamp Low
FastBridgeV2.canClaim(bytes32,address) uses timestamp for comparisons
Dangerous comparisons: - _timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) > DISPUTE_PERIOD |
||
|
||
/// @inheritdoc IFastBridgeV2 | ||
function relay(bytes memory request, address relayer) public payable { | ||
if (relayer == address(0)) revert ZeroAddress(); | ||
|
@@ -178,18 +204,6 @@ | |
); | ||
} | ||
|
||
/// @inheritdoc IFastBridgeV2 | ||
function bridgeRelays(bytes32 transactionId) public view returns (bool) { | ||
// has this transactionId been relayed? | ||
return bridgeRelayDetails[transactionId].relayer != address(0); | ||
} | ||
|
||
/// @inheritdoc IFastBridge | ||
function prove(bytes memory request, bytes32 destTxHash) external { | ||
bytes32 transactionId = keccak256(request); | ||
prove(transactionId, destTxHash, msg.sender); | ||
} | ||
|
||
/// @inheritdoc IFastBridgeV2 | ||
function prove(bytes32 transactionId, bytes32 destTxHash, address relayer) public onlyRole(RELAYER_ROLE) { | ||
// update bridge tx status given proof provided | ||
|
@@ -202,30 +216,6 @@ | |
emit BridgeProofProvided(transactionId, relayer, destTxHash); | ||
} | ||
|
||
/// @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 | ||
/// proof.timestamp < type(uint40).max via unchecked statement | ||
/// @param proofBlockTimestamp The bridge proof block timestamp | ||
/// @return delta Time delta since proof submitted | ||
function _timeSince(uint40 proofBlockTimestamp) internal view returns (uint256 delta) { | ||
unchecked { | ||
delta = uint40(block.timestamp) - proofBlockTimestamp; | ||
} | ||
} | ||
|
||
/// @inheritdoc IFastBridge | ||
function canClaim(bytes32 transactionId, address relayer) external view returns (bool) { | ||
if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); | ||
if (bridgeTxDetails[transactionId].proofRelayer != relayer) revert SenderIncorrect(); | ||
return _timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) > DISPUTE_PERIOD; | ||
} | ||
|
||
/// @inheritdoc IFastBridgeV2 | ||
function claim(bytes memory request) external { | ||
claim(request, address(0)); | ||
} | ||
|
||
/// @inheritdoc IFastBridge | ||
function claim(bytes memory request, address to) public { | ||
bytes32 transactionId = keccak256(request); | ||
|
@@ -258,47 +248,57 @@ | |
emit BridgeDepositClaimed(transactionId, bridgeTxDetails[transactionId].proofRelayer, to, token, amount); | ||
} | ||
|
||
/// @inheritdoc IFastBridge | ||
function dispute(bytes32 transactionId) external onlyRole(GUARD_ROLE) { | ||
if (bridgeTxDetails[transactionId].status != BridgeStatus.RELAYER_PROVED) revert StatusIncorrect(); | ||
if (_timeSince(bridgeTxDetails[transactionId].proofBlockTimestamp) > DISPUTE_PERIOD) { | ||
revert DisputePeriodPassed(); | ||
} | ||
function bridgeStatuses(bytes32 transactionId) public view returns (BridgeStatus status) { | ||
return bridgeTxDetails[transactionId].status; | ||
} | ||
|
||
// @dev relayer gets slashed effectively if dest relay has gone thru | ||
bridgeTxDetails[transactionId].status = BridgeStatus.REQUESTED; | ||
bridgeTxDetails[transactionId].proofRelayer = address(0); | ||
bridgeTxDetails[transactionId].proofBlockTimestamp = 0; | ||
bridgeTxDetails[transactionId].proofBlockNumber = 0; | ||
function bridgeProofs(bytes32 transactionId) public view returns (uint96 timestamp, address relayer) { | ||
timestamp = bridgeTxDetails[transactionId].proofBlockTimestamp; | ||
relayer = bridgeTxDetails[transactionId].proofRelayer; | ||
} | ||
|
||
emit BridgeProofDisputed(transactionId, msg.sender); | ||
/// @inheritdoc IFastBridgeV2 | ||
function bridgeRelays(bytes32 transactionId) public view returns (bool) { | ||
// has this transactionId been relayed? | ||
return bridgeRelayDetails[transactionId].relayer != address(0); | ||
} | ||
|
||
/// @inheritdoc IFastBridge | ||
function refund(bytes memory request) external { | ||
bytes32 transactionId = keccak256(request); | ||
|
||
BridgeTransaction memory transaction = getBridgeTransaction(request); | ||
|
||
if (bridgeTxDetails[transactionId].status != BridgeStatus.REQUESTED) revert StatusIncorrect(); | ||
function getBridgeTransaction(bytes memory request) public pure returns (BridgeTransaction memory) { | ||
return abi.decode(request, (BridgeTransaction)); | ||
} | ||
|
||
if (hasRole(REFUNDER_ROLE, msg.sender)) { | ||
// Refunder can refund if deadline has passed | ||
if (block.timestamp <= transaction.deadline) revert DeadlineNotExceeded(); | ||
/// @notice Pulls a requested token from the user to the requested recipient. | ||
/// @dev Be careful of re-entrancy issues when msg.value > 0 and recipient != address(this) | ||
function _pullToken(address recipient, address token, uint256 amount) internal returns (uint256 amountPulled) { | ||
if (token != UniversalTokenLib.ETH_ADDRESS) { | ||
token.assertIsContract(); | ||
// Record token balance before transfer | ||
amountPulled = IERC20(token).balanceOf(recipient); | ||
// Token needs to be pulled only if msg.value is zero | ||
// This way user can specify WETH as the origin asset | ||
IERC20(token).safeTransferFrom(msg.sender, recipient, amount); | ||
// Use the difference between the recorded balance and the current balance as the amountPulled | ||
amountPulled = IERC20(token).balanceOf(recipient) - amountPulled; | ||
} else { | ||
// Permissionless refund is allowed after REFUND_DELAY | ||
if (block.timestamp <= transaction.deadline + REFUND_DELAY) revert DeadlineNotExceeded(); | ||
// Otherwise, we need to check that ETH amount matches msg.value | ||
if (amount != msg.value) revert MsgValueIncorrect(); | ||
// Transfer value to recipient if not this address | ||
if (recipient != address(this)) token.universalTransfer(recipient, amount); | ||
// We will forward msg.value in the external call later, if recipient is not this contract | ||
amountPulled = msg.value; | ||
} | ||
} | ||
|
||
// if all checks passed, set to REFUNDED status | ||
bridgeTxDetails[transactionId].status = BridgeStatus.REFUNDED; | ||
|
||
// transfer origin collateral back to original sender | ||
address to = transaction.originSender; | ||
address token = transaction.originToken; | ||
uint256 amount = transaction.originAmount + transaction.originFeeAmount; | ||
token.universalTransfer(to, amount); | ||
|
||
emit BridgeDepositRefunded(transactionId, to, token, amount); | ||
/// @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 | ||
/// proof.timestamp < type(uint40).max via unchecked statement | ||
/// @param proofBlockTimestamp The bridge proof block timestamp | ||
/// @return delta Time delta since proof submitted | ||
function _timeSince(uint40 proofBlockTimestamp) internal view returns (uint256 delta) { | ||
unchecked { | ||
delta = uint40(block.timestamp) - proofBlockTimestamp; | ||
} | ||
} | ||
} |
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.
Let's use named variables syntax here (and in relay/claim) for easier maintainability. This wasn't really required when the overload function was literally a few lines away, but with the function ordering enforced that would be helpful: