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.

Sign up for GitHub

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

Jump to bottom

Cross-chain transfers can be frontrun with fee change #60

Closed
code423n4 opened this issue Mar 15, 2022 · 3 comments
Closed

Cross-chain transfers can be frontrun with fee change #60

code423n4 opened this issue Mar 15, 2022 · 3 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists invalid This doesn't seem right sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/db8a1fdddd02e8cc209a4c73ffbb3de210e4a81a/contracts/hyphen/token/TokenManager.sol#L44

Vulnerability details

Impact

Owners can change the fee that is taken on each transfer by calling changeFee.
They can frontrun cross-chain transfers and steal the user's transfer amount.

POC

  • user checks transfer fees on chain Y. They are okay with it and initiate x-chain transfer from X to Y
  • Owner calls changeFee(_equilibriumFee=100%, maxFee=100%)
  • when the transfer is resolved on chain Y by calling sendFundsToUser, the user loses all their funds.

Recommended Mitigation Steps

Consider adding a delay to when the new fees can be activated to ensure that users get the fees they saw when they started the x-chain transfer.
@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Mar 15, 2022
code423n4 added a commit that referenced this issue Mar 15, 2022
@code423n4
cmichel issue #60
e9d9ec0
@ankurdubey521
Copy link
Collaborator

I agree this is an issue, but in the current iteration of Hyphen it is still a centralized system, therefore there is an implicit trust in the contract owners and executors. A decentralized version of the Hyphen bridge is in the works and will fix these issues.

@ankurdubey521 ankurdubey521 added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Mar 30, 2022
@pauliax
Copy link
Collaborator

pauliax commented Apr 30, 2022

#80 (comment)

@pauliax pauliax closed this as completed Apr 30, 2022
@pauliax pauliax added the duplicate This issue or pull request already exists label Apr 30, 2022
@pauliax
Copy link
Collaborator

pauliax commented May 18, 2022

Grouping this together with a similar issue submitted by the same warden: #58

@pauliax pauliax added the invalid This doesn't seem right label May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists invalid This doesn't seem right sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ankurdubey521 @pauliax @code423n4