Open hats-bug-reporter[bot] opened 1 month ago
Good catch!
@rotcivegaf @party-for-illuminati
There is protection by owner for this relayer issue. check here.
function toggleRelayersWhitelistEnabled() public onlyOwner {
relayersWhitelistEnabled = !relayersWhitelistEnabled;
}
function toggleRelayer(address _relayer) public onlyOwner {
_toggleRelayer(_relayer);
}
Shouldn't relayers be trusted?
@rotcivegaf @party-for-illuminati
There is protection by owner for this relayer issue. check here.
function toggleRelayersWhitelistEnabled() public onlyOwner { relayersWhitelistEnabled = !relayersWhitelistEnabled; } function toggleRelayer(address _relayer) public onlyOwner { _toggleRelayer(_relayer); }
The issue is valid because VaultWallet doesn't proxy ownership of serializers to the owner
Shouldn't relayers be trusted?
Yes they should, however it shouldn't be the case if the relayer private key is compromised then a pending withdrawal is broken
@rotcivegaf @party-for-illuminati
There is protection by owner for this relayer issue. check here.
function toggleRelayersWhitelistEnabled() public onlyOwner { relayersWhitelistEnabled = !relayersWhitelistEnabled; } function toggleRelayer(address _relayer) public onlyOwner { _toggleRelayer(_relayer); }
The issue is valid because VaultWallet doesn't proxy ownership of serializers to the owner
Owner can remove the relayer right?
@rotcivegaf @party-for-illuminati There is protection by owner for this relayer issue. check here. AllowedRelayers.sol#L27-L33
function toggleRelayersWhitelistEnabled() public onlyOwner { relayersWhitelistEnabled = !relayersWhitelistEnabled; } function toggleRelayer(address _relayer) public onlyOwner { _toggleRelayer(_relayer); }
The issue is valid because VaultWallet doesn't proxy ownership of serializers to the owner
Owner can remove the relayer right?
Yes but the issue is the ownership isn't being proxied from VaultWallet to the owner
@rotcivegaf @party-for-illuminati There is protection by owner for this relayer issue. check here. AllowedRelayers.sol#L27-L33
function toggleRelayersWhitelistEnabled() public onlyOwner { relayersWhitelistEnabled = !relayersWhitelistEnabled; } function toggleRelayer(address _relayer) public onlyOwner { _toggleRelayer(_relayer); }
The issue is valid because VaultWallet doesn't proxy ownership of serializers to the owner
Owner can remove the relayer right?
Yes but the issue is the ownership isn't being proxied from VaultWallet to the owner
Now, I can see the issue. the transfer ownership tricks the issue
Can i say the same thing about owner being malicious due to private keys lost? @party-for-illuminati
Shouldn't relayers be trusted?
Yes they should, however it shouldn't be the case if the relayer private key is compromised then a pending withdrawal is broken
IMO, by Hats rule, private key compromised is not a valid attack vector to receive a bounty.
LIMITATIONS: Reporters will not receive a bounty for:
- Attacks that require access to leaked private keys or trusted addresses.
@party-for-illuminati @rotcivegaf
Sponsor can accept (acknowledge) and implement fix, but per Hats rules, it will not receive a bounty
@rotcivegaf @party-for-illuminati
when this https://github.com/hats-finance/illuminex-0x0bb4aa1f58719707405c231fcdf0b405714799cf/issues/49 is fixed. do you still issue in this one ?
Github username: -- Twitter username: -- Submission hash (on-chain): 0xb06ad5822293a8d320d028d2c1e10cb09b042bd865af27b3bcd9fc546d8be8f3 Severity: medium
Description: Description\ The
startOutgoingTxSerializing
function is called by a relayer of theVaultBitcoinWallet
contract to start the process of withdrawal for pending withdrawals from the queue. This function deploys a newTxSerializer
contract using theTxSerializerFactory
. The owner of theTxSerializer
contract is transferred to theVaultBitcoinWallet
, which sets the relayer who called thestartOutgoingTxSerializing
function as the only relayer of theTxSerializer
. This means ONLY the relayer who started the process can now interact with the TxSerializer, and make it reach aFinished
seriailization state. So while the next step on theVaultBitcoinWallet
, which is thefinaliseOutgoingTxSerializing
, can be called by any relayer, the_sr.getRaw()
function call it makes will revert unless the original relayer makes the necessary calls to finalize the serialization. This means that in case the original relayer who calledstartOutgoingTxSerializing
does not finilaze the serialization process, the withdrawal mechanism will remain stuck indefinetly, with no way to recovery - essentially giving an absolute power over the system to every individual relayer, requiring more trust in the relayers than what seems intended.There also seems to be no reason for this excessive power to be granted to a single relayer, since either toggling more relayers, or (probably better) giving the ownership over the
TxSerializer
to the owner of theVaultBitcoinWallet
could remove this power from the individual relayer.Attack Scenario\ The above means that every single relayer has the power to stop the system's withdrawals indefinetly.
In case a relayer key is malicious or gets compromised, the attacker can disable withdrawals from the system, and demand a ransom to re-enable them.
In case an honest relayer who initiated the serialization process loses the key before the serialization process is finished (for example a hardware issue causes the key to be lost), the system's withdrawal mechanism will simply remain stuch forever, which no option to recover.
Attachments
Proof of Concept (PoC) File The issue concerns the unnecessarily excessive power given to a single relayer account, not an external attack.
Revised Code File (Optional)
Add this as line 287 of
VaultBitcoinWallet.sol
:_sr.transferOwner(owner());