Check for re-entrancy issues inside sputnik

near-daos / sputnik-dao-contract

Smart contracts for https://app.astrodao.com

https://astrodao.com/

MIT License

108 stars 77 forks source link

Check for re-entrancy issues inside sputnik #98

Closed ctindogaru closed 2 years ago

ctindogaru commented 2 years ago

This is not urgent, but still needs investigation. Check that we don't end up in the following situation:

Since the cross contract calls and the callbacks are async on near, it's very possible that you run into a situation where you claim funds from the DAO multiple times and you'll get the funds each time (instead of getting an error on the second claim). This is because the callback that tells you that the transfer is successful happens long after the actual transfer. So in theory, you can span 10 transactions of the same type until you get the result from the first transaction (if it's successful or not). Conclusion? You could claim and receive 10 times the funds that you should.

gagdiez commented 2 years ago

@ctindogaru @TrevorJTClarke I made this tool some time ago, it may be helpful to test such scenarios: https://github.com/gagdiez/reentrancy_attack

TrevorJTClarke commented 2 years ago

@ctindogaru @TrevorJTClarke I made this tool some time ago, it may be helpful to test such scenarios: https://github.com/gagdiez/reentrancy_attack

Wow sounds awesome - ill take a look! At minimum, worth utilizing to test templating

gagdiez commented 2 years ago

@TrevorJTClarke I am using this repo to deploy a DAO for my project, I can test it for us all. What function do you have doubts about?

Bountydone? Transfer?

Let me know and I'll check it.

TrevorJTClarke commented 2 years ago

@gagdiez I would expect coverage of all actions that have negative impact on governance or collateral. So it needs consideration for the impact of re-entrancy from governance changes in addition to tokens. The following may be overkill, but here's my list:

[ ] ProposalKind::Transfer (NEAR)
[ ] ProposalKind::Transfer (Fungible Token)
[ ] ProposalKind::Transfer (Non Fungible Token)
[ ] ProposalKind::BountyDone
[ ] ProposalKind::ChangeConfig
[ ] ProposalKind::ChangePolicy
[ ] ProposalKind::ChangePolicyAddOrUpdateRole
[ ] ProposalKind::ChangePolicyRemoveRole
[ ] ProposalKind::ChangePolicyUpdateDefaultVotePolicy
[ ] ProposalKind::ChangePolicyUpdateParameters
[ ] ProposalKind::FunctionCall

ctindogaru commented 2 years ago

I guess if Halborn didn't report any re-entrancy issues in their audit report, we can close this.