Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KupiaSec - Protocol incompatibility with smart contract wallets #77

Closed
sherlock-admin3 opened this issue Sep 9, 2024 · 0 comments
Closed
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A Medium severity issue. Reward A payout will be made for this issue

Comments

@sherlock-admin3
Copy link
Contributor

sherlock-admin3 commented Sep 9, 2024

KupiaSec

Medium

Protocol incompatibility with smart contract wallets

Summary

Because of using tx.origin throughout the protocol, it limits the functionality of the protocol to EOA wallets without supporting Safe Wallets or other Smart Wallets.

Vulnerability Detail

In every single external function of the core contract, it uses tx.origin as user address, as follows:

@external
def mint(
  id          : uint256,
  base_token  : address,
  quote_token : address,
  lp_token    : address,
  base_amt    : uint256,
  quote_amt   : uint256,
  ctx         : Ctx) -> uint256:

  self._INTERNAL()

  user        : address   = tx.origin

  ...

Using tx.origin as user address does limit functionality of the protocol because it does not support smart contract wallets.

Impact

Limitation of the protocol functionality of only supporting EOAs.

Code Snippet

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

Tool used

Manual Review

Recommendation

Instead of using tx.origin as user address, the api contract should pass the user address to the core contract, for example as follows:

# api.vy

@external
def mint(
  base_token  : address, #ERC20
  quote_token : address, #ERC20
  lp_token    : address, #ERC20Plus
  base_amt    : uint256,
  quote_amt   : uint256,
  desired     : uint256,
  slippage    : uint256,
  payload     : Bytes[224]
) -> uint256:
  """
  @notice            Provide liquidity to the pool
  @param base_token  Token representing the base coin of the pool (e.g. BTC)
  @param quote_token Token representing the quote coin of the pool (e.g. USDT)
  @param lp_token    Token representing shares of the pool's liquidity
  @param base_amt    Number of base tokens to provide
  @param quote_amt   Number of quote tokens to provide
  @param desired     Price to provide liquidity at (unit price using onchain
                     representation for quote_token, e.g. 1.50$ would be
                     1500000 for USDT with 6 decimals)
  @param slippage    Acceptable deviaton of oracle price from desired price
                     (same units as desired e.g. to allow 5 cents of slippage,
                     send 50000).
  @param payload     Signed Redstone oracle payload
  """
  ctx: Ctx = self.CONTEXT(base_token, quote_token, desired, slippage, payload)
- return self.CORE.mint(1, base_token, quote_token, lp_token, base_amt, quote_amt, ctx)
+ return self.CORE.mint(msg.sender, 1, base_token, quote_token, lp_token, base_amt, quote_amt, ctx)


# core.vy
@external
def mint(
+ user        : address,
  id          : uint256,
  base_token  : address,
  quote_token : address,
  lp_token    : address,
  base_amt    : uint256,
  quote_amt   : uint256,
  ctx         : Ctx) -> uint256:

  self._INTERNAL()

- user        : address   = tx.origin
  total_supply: uint256   = ERC20(lp_token).totalSupply()
  pool        : PoolState = self.POOLS.lookup(id)
  lp_amt      : uint256   = self.POOLS.calc_mint(id, base_amt, quote_amt, total_supply, ctx)

  assert pool.base_token  == base_token , ERR_PRECONDITIONS
  assert pool.quote_token == quote_token, ERR_PRECONDITIONS
  assert pool.lp_token    == lp_token   , ERR_PRECONDITIONS
  assert base_amt > 0 or quote_amt > 0  , ERR_PRECONDITIONS
  assert lp_amt > 0                     , ERR_PRECONDITIONS

  assert ERC20(base_token).transferFrom(user, self, base_amt, default_return_value=True), "ERR_ERC20"
  assert ERC20(quote_token).transferFrom(user, self, quote_amt, default_return_value=True), "ERR_ERC20"
  assert ERC20Plus(lp_token).mint(user, lp_amt), "ERR_ERC20"

  self.POOLS.mint(id, base_amt, quote_amt)
  self.FEES.update(id)

  self.INVARIANTS(id, base_token, quote_token)

  log Mint(user, ctx, pool, total_supply, lp_amt, base_amt, quote_amt)

  return lp_amt

Duplicate of #82

@github-actions github-actions bot added Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A Medium severity issue. labels Sep 11, 2024
@sherlock-admin3 sherlock-admin3 changed the title Dancing Topaz Perch - Protocol incompatibility with smart contract wallets KupiaSec - Protocol incompatibility with smart contract wallets Sep 11, 2024
@sherlock-admin3 sherlock-admin3 added the Reward A payout will be made for this issue label Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A Medium severity issue. Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

1 participant