File(s): DefxBridge.sol
Description:
The cancelValidatorSetUpdate(...) function enables the cancellation of a proposed validator set. This function takes the nonce and the validators’ signatures as inputs. The signed message by the validators includes the nonce and the string "cancelValidatorSetUpdate".
However, the nonce is not incremented after the function call. This oversight facilitates a signature replay attack, allowing an attacker to cancel the proposed validator set repeatedly. If the validators have ever canceled a proposed set at least once, an attacker can exploit this by continuously submitting the same nonce and signatures to block validator updates.
function cancelValidatorSetUpdate(
uint64 nonce,
Signature[] calldata signatures
) external whenNotPaused {
//@audit-issue nonce is not updated
bytes32 messageHash = SignatureLibrary.generateUniqueMessageHash(
keccak256(abi.encode("cancelValidatorSetUpdate", nonce)),
address(this)
);
// ...
}Recommendation(s):
Implement the _verifyAndIncrementNonce(...) function to ensure the usage of a unique nonce within the signed message.
File(s): DefxBridge.sol
Description:
The contract can be paused using the pause() function, which requires a minimum number of lockers (lockerThreshold) to vote for the action. Each time pause() is invoked, the locker address is added to the lockersVotingLock array. When the array reaches the specified threshold, the contract is paused.
To unpause the contract, lockers must call the unpause() function. When a locker invokes this function, its address is removed from the lockersVotingLock array. The contract will unpause if the length of the array falls below lockerThreshold, as illustrated in the code below.
However, this mechanism does not ensure that there is a minimal number of locker votes to unpause the contract.
For instance, if lockerThreshold is set to 5, meaning that five lockers are required to pause the contract, it suffices for just one locker to call unpause() to unlock it. Upon a successful call, that locker’s address is removed from the lockersVotingLock array, resulting in a length of 4. Consequently, the condition (lockersVotingLock.length < lockerThreshold) evaluates to true, leading to the contract being unpaused.
function unpause() external whenPaused {
// ...
for (uint64 i = 0; i < lockersVotingLock.length; i++) {
if (lockersVotingLock[i] == msg.sender) {
lockersVotingLock[i] = lockersVotingLock[
lockersVotingLock.length - 1
];
//@auditlocked removed from the array after voting
lockersVotingLock.pop();
break;
}
}
//@audit Does not require the entire `lockersVotingLock` set to vote to unpause the contract
if (lockersVotingLock.length < lockerThreshold) {
_unpause();
emit ContractResumed();
}
}Recommendation(s):
Ensure that at least lockerThreshold lockers must vote in order to unpause the contract. This can be achieved by checking if the length of the lockersVotingLock array reaches zero. Note that this approach is effective only if the whenNotPaused modifier is added to the pause() function to prevent potential DoS attacks. Alternatively, maintain an internal count of votes to manage the unpausing process.
File(s): Utils.sol
Description:
The isTransactionInDisputeWindow(...) function determines whether a withdrawal request or a validator set update is within the dispute window. This window allows validators to cancel the request and prevent its execution if necessary.
When withdrawal requests are initiated, the current block.timestamp is stored in the requestedTime variable associated with the withdrawal request. Upon finalization of the withdrawal, the system checks whether it is still within the dispute window by passing requestedTime as the time parameter to the isTransactionInDisputeWindow(...) function.
The time parameter is intended to be in milliseconds to ensure the correct functioning of the isTransactionInDisputeWindow(...) function. However, block.timestamp returns the time in seconds, leading to requestedTime being stored in seconds. This discrepancy in units results in an incorrect evaluation of the dispute window.
The same issue applies to the validator set update, as the epochTimestamp parameter refers to a timestamp in seconds.
function isTransactionInDisputeWindow(
uint64 time,
uint64 blockNumber,
uint64 disputePeriodSeconds,
uint64 blockDurationMillis
) internal view returns (bool) {
bool enoughTimePassed = block.timestamp * 1000 >
time + disputePeriodSeconds * 1000; // @audit `time` must be in milliseconds, but is currently in seconds
if (!enoughTimePassed) {
return true;
}
// ...
}Recommendation(s):
Modify the time parameter to use milliseconds consistently or adjust the calculations to use seconds instead of milliseconds throughout the function.
File(s): DefxBridge.sol
Description:
The pause(...) function enables lockers to vote to pause the contract. Once the number of pause votes reaches the defined threshold, the contract is paused.
However, the function lacks the whenNotPaused modifier. As a result, it can be invoked even when the contract has already been paused. This allows additional locker votes to accumulate in the lockersVotingLock array, potentially preventing the contract from being unpaused.
function pause() external {
// ...
//@audit locker is pushed to the array
lockersVotingLock.push(msg.sender);
// ...
//@audit: Function does not revert if already paused
if (lockersVotingLock.length >= lockerThreshold && !paused()) {
_pause();
emit ContractPaused();
}
}Recommendation:
Consider adding the whenNotPaused modifier to the pause function to ensure it cannot be called when the contract is already paused.
File(s): DefxBridge.sol
Description:
The _updateValidatorSet function updates the lockers mapping by adding new validators and removing old ones.
However, an issue arises due to the order of operations. If a validator from the old set is meant to be retained in the new set, it will unintentionally be excluded because the removal of old validators occurs after the new validators have been added.
function _updateValidatorSet(...) internal {
// ...
// Add new validators to lockers
for (uint64 i = 0; i < validatorsColdWallets.validators.length; i++) {
lockers[validatorsColdWallets.validators[i]] = true;
}
// Remove old validators from lockers
//@audit Validator cannot be carried over from the old set to the new set
//because it will be deactivated here
for (uint64 i = 0; i < oldValidatorsColdWallets.validators.length; i++) {
lockers[oldValidatorsColdWallets.validators[i]] = false;
}
// ...
}Recommendation(s):
Reverse the order of operations. First, remove the old validators from the lockers mapping, and then add the new validators to ensure that validators retained from the old set remain active.
File(s): DefxBridge.sol
Description:
The _updateValidatorSet function updates the lockers mapping by adding new validators and removing old ones.
However, an issue arises due to the order of operations. If a validator from the old set is meant to be retained in the new set, it will unintentionally be excluded because the removal of old validators occurs after the new validators have been added.
function _updateValidatorSet(...) internal {
// ...
// Add new validators to lockers
for (uint64 i = 0; i < validatorsColdWallets.validators.length; i++) {
lockers[validatorsColdWallets.validators[i]] = true;
}
// Remove old validators from lockers
//@audit Validator cannot be carried over from the old set to the new set
//because it will be deactivated here
for (uint64 i = 0; i < oldValidatorsColdWallets.validators.length; i++) {
lockers[oldValidatorsColdWallets.validators[i]] = false;
}
// ...
}Recommendation(s):
Reverse the order of operations. First, remove the old validators from the lockers mapping, and then add the new validators to ensure that validators retained from the old set remain active.
File(s): DefxBridge.sol
Description:
The batchDepositWithPermit(...) function utilizes a permit(...) function that lacks consistency across various blockchain networks. This inconsistency can lead to functionality issues when interacting with certain ERC20 tokens.
For instance, consider the DAI token on Ethereum and Arbitrum networks, which has different signatures:
- Ethereum Mainnet DAI
function permit(address holder, address spender, uint256 nonce, uint256 expiry, bool allowed, uint8 v, bytes32 r, bytes32 s);
- Arbitrum DAI
function permit(address owner, address spender, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s);
The batchDepositWithPermit(...) function is currently using the Arbitrum version of permit(...). As a result, this implementation is incompatible with the Ethereum Mainnet DAI, where the permit(...) function has a different signature. This discrepancy prevents the batchDepositWithPermit(...) function from operating correctly on Ethereum for DAI tokens. Note that other tokens can also implement the permit(...) function differently.
Recommendation(s):
Ensure the support of the permit(...) signature before allowing a token to be used within the bridge. Alternatively, implement a deposit mechanism where the user can directly deposit tokens without relying on the permit(...) function.