Greed - The use of tx.origin may cause a trader to lose funds

[sherlock-admin4]

Greed

Medium

The use of tx.origin may cause a trader to lose funds

Summary

Traders interact with the protocol through calls to the api.vy contract. Every interaction is then forwarded to the core.vy contract which is responsible for identifying the trader's address, pulling tokens from the address and performing the desired operation.

Vulnerability Detail

The issue comes from the fact that the core.vy contract identifies the trader using tx.origin. This can lead to unexpected and undesirable behaviors especially when using Smart Contract Wallets (Gnosis Safe ; Abstract Accounts ; ...)

https://github.com/sherlock-audit/2024-08-velar-artha/blob/main/gl-sherlock/contracts/core.vy#L241

@external
def open(
  id          : uint256,
  base_token  : address,
  quote_token : address,
  long        : bool,
  collateral0 : uint256,
  leverage    : uint256,
  ctx         : Ctx) -> PositionState:

  self._INTERNAL()

@>  user       : address   = tx.origin

Let the following scenario :

• a trader owns a Smart Contract Wallet, with 3 other traders, that holds 4,000 USDT
• the trader also holds 5,000 USDT
• the trader tries to use the protocol and approve() the core.vy contract to pull type(uint256).max amount of USDT
• the trader opens a position with 100 USDT
• now the traders that own the smart wallet are confident and want to commit 3,000 USDT from their smart wallet
• they all sign the required set of transactions to open() a position in Velar (e.g. USDT.approve())
• the initial trader triggers the set of transactions with the intention to open a position for the smart wallet
• Velar identifies the trader with tx.origin and performs the open() for the trader's address, not the smart wallet

The initial trader commited 3,000 USDT from his own tokens in Velar

He is now subjected to price fluctuation of the market and has an additional position opened, which he did not intend to.

Impact

This design can potentially cause a loss of funds for the protocol users, performing internal operations with the address that initiated the transaction rather than a potential third party contract.

Tool used

Manual Review

Recommendation

In api.vy retrieve msg.sender and forward this address as a new parameter in all core.vy functions.

For example :

api.vy

@external
def open(
    base_token  : address,
    quote_token : address,
    long        : bool,
    collateral0 : uint256,
    leverage    : uint256, # @audit does setting it to 0 break something? A : can't set it to 0
    desired     : uint256,
    slippage    : uint256,
    payload     : Bytes[224]
) -> PositionState:

    ctx: Ctx = self.CONTEXT(base_token, quote_token, desired, slippage, payload)
-   return self.CORE.open(1, base_token, quote_token, long, collateral0, leverage, ctx)
+   return self.CORE.open(1, base_token, quote_token, long, collateral0, leverage, msg.sender, ctx)

core.vy

@external
def open(
    id          : uint256,
    base_token  : address,
    quote_token : address,
    long        : bool,
    collateral0 : uint256,
    leverage    : uint256,
+   user        : address,
    ctx         : Ctx) -> PositionState:

    self._INTERNAL()
-   user       : address   = tx.origin

Duplicate of #82