Conversation continued (see below), but not a critical issue for my contract as execution is not open, and the entity with the executor role and those in control of it (multisig owners) are fully trusted.
Damian, [31.08.21 13:25]
- Yes, 4.3.1 is fine.
- Yep, we are in contact, because they asked me to hold on the publication of the article regarding this (I can share draft if you want, but please do not send it further).
- Yep, https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-fg47-3c2x-m2wr
- It's OZ's code. Upgrade fixes it (details are in my article, see above 2).
- Im not aware of any or when I find it I report (like I did with you) and do not spread it. I can share a PoC if you want.
- I am a security researcher and bug bounty hunter. You can check my presentation on EthCC (https://www.youtube.com/watch?v=e_2A9bNgW24). I have not met your project before.
- Not following Check-Effects-Interactions patter (details in the article, I guess you would be interested in that). OZ's postmortem is going public shortly as well.
- Using the delay bypass and privilege escalation in OZ timelock I can control your owner and call changeFeePercent function to make fee be 100%. Then I would also revoke your roles in TimelockController so that you would not be able to bring it back.
Damian, [31.08.21 13:28]
- What do you mean by motivation? I am security researcher and ethical hacker (check out my Twitter and EthCC presentation :)). If I hunt a bug I report it to get bounty and I also do security audits.
- Critical bugs, which I would say this is, do not usually have lower bound and are "up to"s. I usually get from 50k USD for critical ones, but I do not know the nature of your project.
Neo, [31.08.21 15:00]
SchnoodleGovernance is not ownable. It implements
AccessControl. So, by "control your owner", do you mean change the admin role?
Damian, [31.08.21 15:04]
By owner I mean V3 token’s owner which is Governance.
I could call this: https://github.com/schnoodledefi/contracts/blob/main/contracts/SchnoodleV3.sol#L90
Via this https://github.com/schnoodledefi/contracts/blob/main/contracts/SchnoodleGovernance.sol
Neo, [31.08.21 15:10]
How would you escalate privileges without the executor role?
Damian, [31.08.21 15:11]
This vuln requires executor role if the execution is not open.
Here is the modifier: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/governance/TimelockController.sol#L95
In your case its not open so the attacker must have the executor role.
Neo, [31.08.21 15:14]
Indeed. That's what I thought. So, the contract is not vulnerable.
Damian, [31.08.21 15:15]
Yes, it is. Only the condition change. Attacker must be executor. But you are right it is not critical. I should have checked that its not open.
Neo, [31.08.21 15:16]
I fully trust those who are executors, so definitely not critical. But thank you for pointing this out, and also for your honesty. I would be happy to consider you working with us if there is the option.
Damian, [31.08.21 15:18]
I understand, but full trust is what you want to avoid in Ethereum If you have full trust, do not use blockchain But on the other hand, such architecture can lead to potential suspicion of rug pulls. I really advise to update it.
Neo, [31.08.21 15:19]
OK, let me explain. I fully trust the executor because it is another contract subject to multisig. This is all addressed in our whitepaper under 'Multisig'.
Damian, [31.08.21 15:20]
Ok, got it. Advice still holds anyway It allows to make it fully trustless.
I am auditor working for a security company. We could cooperate if you need an audit but I am not looking forward to join a team as a member.
Sorry for a bit of panic
Neo, [31.08.21 15:29]
Yes, it's more trustless than most other projects, but it's a small project (microcap) right now, therefore potentially subject to whale manipulation with the DAO process. This is why it's explained in the whitepaper under the Schnoodle DAO heading why it remains backed up with multisig for now. And there's a further backup with the timelock controller (
SchnoodleGovernance) of course, which means any contract interaction via multisig is still subject to a 24-hour delay allowing holders to take action in advance of any possible rug.
Damian, [31.08.21 15:37]
Yeah, seems good.