makerdao / spells-mainnet

Staging repo for MakerDAO weekly executive spells
GNU Affero General Public License v3.0
122 stars 50 forks source link

Agree on deprecating dapp tools #398

Open SidestreamColdMelon opened 8 months ago

SidestreamColdMelon commented 8 months ago

Discuss strategy of dapp tools deprecation and if we want to do it.

One counter argument: having dapp tools and foundry cross checking the output binary is better than having a single tool to circumvent supply chain attacks. In this case, we can make foundry the main development+deployment tool and use dapp-tools for crosschecking (and not the way around).

SidestreamColdMelon commented 6 months ago

Based on the above, the following would be a proposed plan to migrate from dapp-tools:

amusingaxl commented 6 months ago

I agree with the changes. The only potentially challenging part would be setting up dapp tools in CI for cross-checking.

With that said, dapp tools being basically abandonware nowadays adds more risks than benefits.

Supply chain attacks would most likely be very hard to catch, since MakerDAO is probably the only relevant org that keeps using it. If we do not very thoroughly check the sources from which we pull the executables every single time, it would be very hard to catch a problem. However, this seems like a huge operational burden.

An alternative would be to port the "immutable" part of the flow (i.e.: base tests) to another tool that is still under active maintenance, such as Hardhat, and perform the cross-checks with it.

SidestreamColdMelon commented 6 months ago

dapp tools being basically abandonware nowadays adds more risks than benefits.

I don't think it adds any risks if it's executed within a CI / docker environment and is no longer used for deployment (or otherwise have access to private keys) – this way, dapp-tools would only get access to the spell code which is already available to everyone. The only thing approach adds:

This approach removes the risk of either foundry or dapp-tools going rogue, but not saving us from other supply chain attacks (eg patched solc compiler) – but that wouldn't be achievable even with the proposed Hardhat-based tests.

An alternative would be to port the "immutable" part of the flow (i.e.: base tests) to another tool that is still under active maintenance, such as Hardhat, and perform the cross-checks with it.

I see that this brings a huge overhead on keeping two source codes written in different languages in sync. I can also imagine that hardhat-based tests would be a lot slower.

amusingaxl commented 3 weeks ago

Going back to this after a while.

  • [ ] Extend checklists to enforce using stable foundry version and document used version / logs

This one can be tricky. There's no "stability" in Foundry development yet, it follows the nightly releases approach, so it would be extremely hard to pinpoint a "safe" version.

  • [ ] Add dockerfile + CI workflow to run tests using dapp-tools

This is also tricky because the current tests depend on Foundry and don't work on Dapp Tools anymore because we are using Foundry-specific features.


In the end of the day, I become more and more skeptical of the benefits that cross-checking brings. I am not aware of any other project in this industry that does that.

If we fear that supply chain attacks might occur, we can come up with a "dev manual" on how to deal with this (i.e.: only install Foundry from official sources), so we can minimize the risk.

SidestreamColdMelon commented 5 days ago

Thanks for reviving this conversation!

  • [ ] Extend checklists to enforce using stable foundry version and document used version / logs

This one can be tricky. There's no "stability" in Foundry development yet, it follows the nightly releases approach, so it would be extremely hard to pinpoint a "safe" version.

Looking at how situation went with the last big supply chain attack using Ledger library, it was detected by the community fairly quickly. So to be on the safe side, we can add a requirement to checklist to use a foundry version that is, for example, exactly one week old + document which exact version was used. Instead of requiring to use foundryup which pulls the latest version that can be baked a minute ago and immediately have access to our private env.

  • [ ] Add dockerfile + CI workflow to run tests using dapp-tools

This is also tricky because the current tests depend on Foundry and don't work on Dapp Tools anymore because we are using Foundry-specific features.

I agree that it can be a problem. As proposed in the last call, we can then use dapp-tools to only run a test similar to testBytecodeMatches (and only inside the CI).

I am not aware of any other project in this industry that does that.

I don't think it's a valid argument. No one in the industry is using checklists (to my knowledge), but it's a simple and powerful tool used in other industries (namely aviation, healthcare) and we're already using it. Same with cross-compilation: it's a simple technique and we're already using it, so let's maintain it, unless there is a very substantial effort required for this to happen (which is also a valid argument). So to push the agreement forward: I agree to drop cross-compilation, if we try and fail to archive it within reasonable amount of time.

amusingaxl commented 2 days ago

So to be on the safe side, we can add a requirement to checklist to use a foundry version that is, for example, exactly one week old + document which exact version was used. Instead of requiring to use foundryup which pulls the latest version that can be baked a minute ago and immediately have access to our private env.

Agreed. Let's amend the checklists then.

However, I'm not entirely sure how to enforce this 7 days grace period. What happens if someone called foundryup recently and need to revert to a previous version? Is that even possible?

Same with cross-compilation: it's a simple technique and we're already using it, so let's maintain it, unless there is a very substantial effort required for this to happen (which is also a valid argument)

We're not exactly doing cross-compilation here. Both Foundry and Dapp Tools use solc under the hood to compile the contracts.

The only gains the cross-checking brings is if any of those tools manage to successfully replace a valid solc build with a rogue one.

SidestreamColdMelon commented 2 days ago

However, I'm not entirely sure how to enforce this 7 days grace period. What happens if someone called foundryup recently and need to revert to a previous version? Is that even possible?

Yes, it's possible to specify exact foundry version to install using -v/--version flag. So I would propose the following flow:

I can add this to checklists. I also think the crafter should post this comment as well as the reviewers (which is potentially even more important, since the deployment is done by the crafter).

We're not exactly doing cross-compilation here.

Technically yes. If solc itself is compromised, that wouldn't be detected in the proposed solution. But we're not updating solc version every two weeks. In fact, we didn't bump the version with which spells are being compiled for at least 2 years. So this risk is minimal. It's more likely that the supply chain attack will target dapp-tools or foundry.

amusingaxl commented 2 days ago

Sounds good. Would you please update the checklists then? I'll look into how we can run testBytecodeMatches with dapp tools from CI.