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

ctf_sec - Should not use of tx.origin track user address #135

Closed
sherlock-admin4 opened this issue Sep 9, 2024 · 0 comments
Closed

ctf_sec - Should not use of tx.origin track user address #135

sherlock-admin4 opened this issue Sep 9, 2024 · 0 comments
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-admin4
Copy link
Contributor

sherlock-admin4 commented Sep 9, 2024

ctf_sec

Medium

Should not use of tx.origin track user address

Summary

the code use tx.origin to track the user address

Vulnerability Detail

example:

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

user       : address   = tx.origin
  pool       : PoolState = self.POOLS.lookup(id)

  cf         : Fee       = self.PARAMS.static_fees(collateral0)
  fee        : uint256   = cf.fee
  collateral : uint256   = cf.remaining

  assert pool.base_token  == base_token , ERR_PRECONDITIONS
  assert pool.quote_token == quote_token, ERR_PRECONDITIONS
  assert collateral > 0                 , ERR_PRECONDITIONS
  assert fee > 0                        , ERR_PRECONDITIONS

  if long: assert ERC20(quote_token).transferFrom(user, self, collateral0), "ERR_ERC20"
  else   : assert ERC20(base_token).transferFrom(user, self, collateral0),  "ERR_ERC20"

  # transfer protocol fees to separate contract
  if long: assert ERC20(quote_token).transfer(self.COLLECTOR, fee), "ERR_ERC20"
  else   : assert ERC20(base_token).transfer(self.COLLECTOR, fee),  "ERR_ERC20"

Impact

multisig wallet has owner alice and bob

alice calls open position using the multisig wallet (genosis safe)

alice calls genosis safe contract calls the velar contract,

while alice expects the user is the safe wallet, alice is tx.origin and she open a position using her own account, not on the safe wallet.

also using tx.origin has other security issue and break all smart contract integration.

https://medium.com/coinmonks/smart-contract-security-tx-origin-authorization-attack-vectors-027730ae601d

and

sherlock-audit/2024-02-optimism-2024-judging#194

Code Snippet

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

Tool used

Manual Review

Recommendation

pass in the original msg.sender inside the core contract instead of tx.origin to track address directly.

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 Wonderful Orchid Dove - Should not use of tx.origin track user address ctf_sec - Should not use of tx.origin track user address 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

2 participants