Broken Functionality and Griefing Vulnerability

Hey, I was looking at the helix smart contracts and I found a broken functionality in the GaurdV3 contract. Also there is a High vulnerability, a griefing attack vector, as well. First I decided to email these issues but then I realized that these contracts are new and are not on-chain, so a GitHub issue would be good. Also the griefing attack is only possible when the broken functionality will be fixed.

Broken Functionality:

The deposit function is the GaurdV3.sol contract, is supposed to deposit tokens from the depositer into the gaurd contract as per the comments & TokenDeposit event. But It doesn't transfers the tokens from the user to the contract but adds the user into the deposits mapping.

    function deposit(uint256 id, address token, address recipient, uint256 amount) public onlyDepositor whenNotPaused {
        deposits[id] = hash(abi.encodePacked(msg.sender, block.timestamp, token, recipient, amount));
        ///@audit-issue tokens not transferred but mapping updated + event emitted
        emit TokenDeposit(msg.sender, id, block.timestamp, token, recipient, amount);
    }

Mitigation:

Mark the deposit function as payable (also add the check to make sure msg.value == amount) and to handle non-native tokens, transferFrom the tokens from the users to the Guard (also make sure in that case msg.value == 0, so the depositer accidentaly don't send any native tokens)

Vulnerablity:

The claimByTimeout function opens a griefing attack vector (Greifing: Attacker will not gain any value but his action can result in a loss for a user) As the claimByTimeout function allows depositors to claim tokens without sending a signature. And the claimByTimeout function internally calls claimById function, claimById function sends the tokens to the users and deletes the deposits[] https://github.com/helix-bridge/contracts/blob/b34abef269a172d8b87a3b17473016ff06e40437/helix-contract/contracts/mapping-token/v3/GuardV3.sol#L97

The problem is that anyone can call claimByTimeout function for anyone else. So an attacker can call that function for a depositor for 1 wei and freeze other tokens of the user inside the contract as the deposits[] will be cleared after 1 wei claim.

Proof-of-Concept:

Ali, a depositor, deposits into the guard 10 WETH.
Time passes, claim time comes
An attacker, Bob, calls the claimByTimeout function for Ali, but with only 1 WETH.
1 WETH gets sent to Ali and his deposits hash is now deleted
Ali's 9 WETH are now freeze into the contract forever, Ali suffers a big loss.

Mitigation:

Make sure the msg.sender is the correct depositer address (which was in the hash) and no one can call claimByTimeout for anyone else.

Thanks! Shaheen

Hey, I was looking at the helix smart contracts and I found a broken functionality in the GaurdV3 contract. Also there is a High vulnerability, a griefing attack vector, as well. First I decided to email these issues but then I realized that these contracts are new and are not on-chain, so a GitHub issue would be good. Also the griefing attack is only possible when the broken functionality will be fixed.

Broken Functionality:

The deposit function is the GaurdV3.sol contract, is supposed to deposit tokens from the depositer into the gaurd contract as per the comments & TokenDeposit event. But It doesn't transfers the tokens from the user to the contract but adds the user into the deposits mapping.
    function deposit(uint256 id, address token, address recipient, uint256 amount) public onlyDepositor whenNotPaused {
        deposits[id] = hash(abi.encodePacked(msg.sender, block.timestamp, token, recipient, amount));
        ///@audit-issue tokens not transferred but mapping updated + event emitted
        emit TokenDeposit(msg.sender, id, block.timestamp, token, recipient, amount);
    }
Mitigation:

Mark the deposit function as payable (also add the check to make sure msg.value == amount) and to handle non-native tokens, transferFrom the tokens from the users to the Guard (also make sure in that case msg.value == 0, so the depositer accidentaly don't send any native tokens)

Vulnerablity:

The claimByTimeout function opens a griefing attack vector (Greifing: Attacker will not gain any value but his action can result in a loss for a user) As the claimByTimeout function allows depositors to claim tokens without sending a signature. And the claimByTimeout function internally calls claimById function, claimById function sends the tokens to the users and deletes the deposits[]

https://github.com/helix-bridge/contracts/blob/b34abef269a172d8b87a3b17473016ff06e40437/helix-contract/contracts/mapping-token/v3/GuardV3.sol#L97

The problem is that anyone can call claimByTimeout function for anyone else. So an attacker can call that function for a depositor for 1 wei and freeze other tokens of the user inside the contract as the deposits[] will be cleared after 1 wei claim.

Proof-of-Concept:

Ali, a depositor, deposits into the guard 10 WETH.

Time passes, claim time comes

An attacker, Bob, calls the claimByTimeout function for Ali, but with only 1 WETH.

1 WETH gets sent to Ali and his deposits hash is now deleted

Ali's 9 WETH are now freeze into the contract forever, Ali suffers a big loss.

Mitigation:

Make sure the msg.sender is the correct depositer address (which was in the hash) and no one can call claimByTimeout for anyone else.

Thanks! Shaheen

Thank you for your research and discovery of the contract. I have analyzed the two issues you listed below:

Does the guard support native tokens? Since the depositor only authorizes guards but does not directly transfer tokens into the guard contract, using payable is not suitable to support native tokens. However, the depositor can indirectly support native tokens by converting them to wrapped tokens. This is currently implemented in the xTokenIssuing contract in Helix.
Anyone can claim any number of tokens via claimByTimeout and delete the ID? Since the deposit_id is a hash consisting of several parameters, including the amount (see guard), if an attacker extracts the token with an incorrect amount, a different deposit_id will be generated. Unless the attacker constructs an amount that collides with the same deposit_id, which is extremely unlikely and considered impossible. This is a very low probability event and is considered impossible.

@xiaoch05, thanks for the review. I see, looks like I made some bad assumptions there.

Anyone can claim any number of tokens via claimByTimeout and delete the ID? Since the deposit_id is a hash consisting of several parameters, including the amount (see guard), if an attacker extracts the token with an incorrect amount, a different deposit_id will be generated. Unless the attacker constructs an amount that collides with the same deposit_id, which is extremely unlikely and considered impossible. This is a very low probability event and is considered impossible.

Quite Interesting!

I agree that issue is not a potential threat, but I still think msg.sender should be checked because a attacker can play with isNative. Maybe the receiver expects Native tokens to receive but an attacker sends non-native tokens or the receiver expects non-native tokens and an attacker sends native tokens. Keeping in mind Frontrun attacks, this is hugely possible. It's a big problem when the receiver is a contract which cannot handle ERC20 tokens, only Ethers but an attacker claimById with isNative being false. It can be problematic in a lot of ways. It's a Dark Forest out there my friend! :)

To transfer the tokens from account A to account B, users can call the transfer() function of the ERC20 token. For example, Alice can call transfer(bob.address,10). This method is suitable and safe for transferring to EOA (Externally Owned Accounts) only. However, if you want to transfer the tokens to the contract, you need to ensure that the receiver contract can handle the incoming transfers–that it is compatible with the ERC20 standard interface. An accidental token transfer to a non-ERC20 contract will result in a loss of tokens. If you transfer the tokens directly to a non-ERC20 contract, then the transfer will be successful, but the tokens are stuck forever in the receiver contract, since it doesn’t have the functionality to move the tokens from the contract.

@xiaoch05, thanks for the review. I see, looks like I made some bad assumptions there.

Anyone can claim any number of tokens via claimByTimeout and delete the ID? Since the deposit_id is a hash consisting of several parameters, including the amount (see guard), if an attacker extracts the token with an incorrect amount, a different deposit_id will be generated. Unless the attacker constructs an amount that collides with the same deposit_id, which is extremely unlikely and considered impossible. This is a very low probability event and is considered impossible.

Quite Interesting!

I agree that issue is not a potential threat, but I still think msg.sender should be checked because a attacker can play with isNative. Maybe the receiver expects Native tokens to receive but an attacker sends non-native tokens or the receiver expects non-native tokens and an attacker sends native tokens. Keeping in mind Frontrun attacks, this is hugely possible. It's a big problem when the receiver is a contract which cannot handle ERC20 tokens, only Ethers but an attacker claimById with isNative being false. It can be problematic in a lot of ways. It's a Dark Forest out there my friend! :)

To transfer the tokens from account A to account B, users can call the transfer() function of the ERC20 token. For example, Alice can call transfer(bob.address,10). This method is suitable and safe for transferring to EOA (Externally Owned Accounts) only. However, if you want to transfer the tokens to the contract, you need to ensure that the receiver contract can handle the incoming transfers–that it is compatible with the ERC20 standard interface. An accidental token transfer to a non-ERC20 contract will result in a loss of tokens. If you transfer the tokens directly to a non-ERC20 contract, then the transfer will be successful, but the tokens are stuck forever in the receiver contract, since it doesn’t have the functionality to move the tokens from the contract.

Yes, this make sense. But we can't restrict the msg.sender to be the depositor, because the depositor is just the provider of the token, and it generally doesn't have a method to execute the claim func, so maybe we need to restrict the msg.sender to be the receiver or some authorised account.

Let's Go! Hope it gets fixed so users funds stay safe :)

Yeah, restricting the msg.sender to be the receiver would be a good idea.

helix-bridge / contracts