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

Audit at 2021-06-08 #7

Merged

Conversation

evgenykuzyakov
Copy link

I haven't looked:

  • fungible_token_standard.rs
  • validator_loans.rs
  • migrations.rs

Comments inline. Major concerns:

  • META withdrawal can be called multiple times before the value is updated.
  • The contract can potentially be locked if something with the callback fails. This can be relaxed using the following pattern invented by @oysterpack. You have schedule 3 promises where one depends on the previous one, let's call them: remote, then, finally. Since both then and finally promises are executed on your contract you guarantee the correctness of the data that you pass there. But you also may let then to fail. Now in the finally promise you unlock the contract and check that then promise succeeded, but if then failed, you rollback the state that you did in the original call.
  • I haven't looked too closely at contract_account_balance, but transfer_extra_balance_accumulated may actually be unsafe if contract_account_balance is incorrectly calculated.
  • Border blocks cases for the epoch may cause some issues. If some operation of this contract called in the last block of the epoch, then the execution on the staking pool may happen in the next epoch. This may lead to attempt at earlier withdrawal.
  • Add liquidity may be potentially wrapped into a sandwich by a malicious validator resulting in getting liquidity at the unfair terms. Or the pool may shift before the liquidity is added. Ideally you want to add some slippage limit on min LP shares amount received.

@luciotato luciotato merged commit 87d1a93 into Narwallets:master Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants