Japy69 - Unsafe use of tx.origin in the open function will lead to unauthorized position closing

[sherlock-admin2]

Japy69

Medium

Unsafe use of tx.origin in the open function will lead to unauthorized position closing

Summary

The use of tx.origin in the close function of the Velar protocol exposes users to phishing attacks. If a user grants unlimited allowance to the contract for base_token or quote_token, a malicious contract can execute the close function on their behalf, resulting in unauthorized closure of leveraged positions. The root cause is the use of tx.origin to determine the user, which is unsafe as it can be manipulated by intermediate contracts.

Root Cause

In core.vy:281, there is an unsafe use of tx.origin, which can lead to unauthorized closing of positions. The choice to use tx.origin might be due to the API.vy contract serving as an intermediary between the user and the core.vy contract. However, tx.origin is considered insecure because it references the original sender of a transaction, which can lead to unintended consequences when intermediary contracts are involved. The Solidity documentation provides a detailed explanation of why tx.origin is unsafe: Solidity Security Considerations - tx.origin.

The close function in core.vy allows users to close their leveraged positions by settling them against the pool. The function uses tx.origin to identify the user initiating the transaction:

user : address = tx.origin

This design is problematic because tx.origin represents the original sender of the transaction, not necessarily the direct caller. In scenarios where a user interacts with another contract that in turn calls the close function, tx.origin will still point to the original user.

Internal Pre-conditions

1. The victim must have granted an allowance to the core contract address for either base_token or quote_token, enabling the contract to transfer tokens on the victim's behalf.

2. The victim must initiate a transaction with a smart contract that allows the attacker to indirectly call the close function at any point during the transaction. This can be easily achieved, for example, if the victim interacts with an automated router like Uniswap's that swaps tokens based on optimal trade routes. The attacker could create a malicious token involved in the trade, and within the transfer function of this malicious token, the attacker can execute a call to the Velor protocol's close function. Because tx.origin will still refer to the victim, the Velor protocol will perceive the transaction as initiated by the victim.

External Pre-conditions

No response

Attack Path

If a user has set an unlimited allowance for base_token or quote_token to this contract, a malicious contract can execute a phishing attack by:

1. Encouraging the user to initiate a transaction on the malicious contract. As explained in internal pre-conditions, it is not hard.
2. Having the malicious contract call the close function on API.vy.
3. Using tx.origin to transfer tokens from the user’s address to the contract, closing a leveraged position without the user's explicit consent.

Impact

• Unauthorized Position Closure: An attacker can exploit this vulnerability to force a user to close their leveraged positions, leading to unauthorized token movements and potential financial losses due to unexpected liquidation or settlement conditions.

PoC

No response

Mitigation

To mitigate this vulnerability, replace tx.origin with a user parameter passed to the close function. This parameter should be set by the msg.sender when the API.vy contract calls the function. The updated function signature should look like this:

@external
def close(
  user        : address,
  id          : uint256,
  base_token  : address,
  quote_token : address,
  position_id : uint256,
  ctx         : Ctx) -> PositionValue:

Additionally, the contract call on api.vy:183 should be updated to:

  return self.CORE.close(msg.sender, 1, base_token, quote_token, position_id, ctx)

By passing msg.sender as the user, the function ensures that only the immediate caller is authorized to initiate the closing of a leveraged position, thus preventing phishing attacks.

Duplicate of #82