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

Incompatability with deflationary / fee-on-transfer tokens #326

Closed
HardlyDifficult opened this issue May 30, 2022 · 2 comments
Closed

Incompatability with deflationary / fee-on-transfer tokens #326

HardlyDifficult opened this issue May 30, 2022 · 2 comments
Labels
invalid This doesn't seem right

Comments

@HardlyDifficult
Copy link
Collaborator

From minhquanym in #95

Incompatability with deflationary / fee-on-transfer tokens
Function Cally.createVault function takes a tokenIdOrAmount parameter but this parameter is not the actual transferred amount for fee-on-transfer / deflationary (or other rebasing) tokens in case tokenType = ERC20
Impact
The actual deposited amount might be lower than the specified depositAmount of the function parameter.
And when users exercise or withdraw they not only receive less than expected amount but also take funds of other vaults with the same vault.token too, causes loss of funds.
Proof-of-concept
https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L200
https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L296
https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L345
Recommended Mitigation Steps
Transfer the tokens first and compare pre-/after token balances to compute the actual amount.

@HardlyDifficult HardlyDifficult added bug Something isn't working duplicate This issue or pull request already exists 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels May 30, 2022
@HardlyDifficult
Copy link
Collaborator Author

Dupe of #61

@JeeberC4 JeeberC4 added invalid This doesn't seem right and removed bug Something isn't working duplicate This issue or pull request already exists 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jun 6, 2022
@JeeberC4
Copy link
Contributor

JeeberC4 commented Jun 6, 2022

Issue recreated with script that includes all required data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

2 participants