Signer Key Bug + Bug Bounty

I briefly touched on the bug during the dev call on Saturday, but here are some more details.

Summary

Basically, the bug was introduced when we added the new InitSpotOpenOrders instruction. I had an assumption hidden through all my code that if you call invoke_signed and none of the passed in pubkeys match the PDA generated by seeds, there would be some kind of error. I patched that assumption by checking for signer_key in every instruction it’s passed in. I don’t think I looked too deeply into that assumption.

Bounty

I think we should go ahead and formally adopt a bug bounty program now. I propose adopting the one Drift has already set up: https://docs.drift.trade/bug-bounty

The one edit I’d make is cap it at $1m instead of 500k as they have done.

The exploit would allow the attacker to steal the entire contents of the vaults which was valued at roughly $180m at the time Justin contacted me. So by the proposed bug bounty program, the bounty would be $1m.

That being said, I think this particular case is a bit weird. I have great respect for Justin finding this bug and reporting it to us promptly so it can be patched. He stayed up late and gave me numerous missed calls to get it done. However, we also paid him 800k MNGO earlier for a full code review which he has not completed due to some personal family issues. So I think we ought to pay out $500k in this case with the understanding that, if it was somebody else, we would pay the full $1m.

Mistakes & Lessons

  • Lesson: don’t make assumptions about solana runtime
  • Lesson: check everything even if it doesn’t need to be checked
  • Mistake: I contacted Neodyme for a full audit of Mango v3, but when they didn’t respond I failed to follow up on it. Definitely going to get a full audit with the hope that no bugs are found and we have the precious fully audited sticker on Mango.

This will be a formal onchain proposal in the next ‘legislative session’. Let the discussion begin.

8 Likes

I think a strong bug bounty is essential.

My only question here is: Was the bug discovered in the process of the code review which Mango paid 800k MNGO for?

If mango pays Neodyme for a full audit of Mango-V3, then they discover a bug that would drain funds, would they be in line for another ‘bug bounty’ on top? That feels a bit too double dippy to me.

Your proposal above essentially accounts for this, but I think it should be clear going into paying someone to review the code if bugs found during that review qualify for the bug bounty, and compensation should take that into account.

2 Likes

Yeah this is a very valid point. Though I should mention that the 800k MNGO was partially as a gift because he found an exploit in Mango v1 when we had no money to pay him. We also had an agreement that he’d do a “full code review” for Mango v3. He started on that, but I think he was knocked out for personal reasons and offered to return the MNGO which I refused.

One further complication, I’m not sure what a “full code review” actually means. Justin seems to think he has not done it yet because it would require understanding a lot of the mechanisms. That’s a bit more thorough than checking the usual things like signer key.

There are some important signaling benefits to encourage people to find bugs, report them and get clean money and reputation. But I take your point that going forward we should be clear on what a review means and when it is started and completed.

Great discussion so far. I think the situation is in a bit of a gray area so I do think some sort of compromise is appropriate but I also want to be clear on how things stand from my perspective.

First off, I’m not a formal auditor and while I’ve helped Mango Markets with reviews in the past, it was always because I care about the success of the protocol because the team, tech, and ideas are really solid. I also like reviewing smart contracts to learn how the Solana runtime and SDK can be safer to help avoid common issues.

A few months ago, I received the 800k MNGO grant which I felt was generous at the time for finding a pre-launch “loss of funds” bug but it was mutually understood that it would make me more invested in the protocol (I’ve never sold) and that I would continue reviewing the code. The scope of my “code review” has always felt uncertain to me because it was never framed as a full audit. Over the next few following weeks I found a number of mild issues that I reported but due to personal issues, was never able to get to a point where I felt confident that there were no serious bugs. I communicated my situation to daffy and even half-joked that I could give back some of the tokens, which never went anywhere. Honestly, I felt guilty that I couldn’t spend more time on it because I wanted to be more helpful. But the ambiguity of the expectations allowed me to disengage and feel okay that I had given as much time as I could under the given circumstances.

Fast forward a few months, my personal matters are almost wrapped up (still ongoing unfortunately), I had a great time getting to know the Mango team in Lisbon, and I was interested in putting more assets into Mango. Before doing so, I decided to take another look and pretty quickly spotted the signer issue. I did a small proof of concept to show that I could create spoofed data and was that was complete, I immediately contacted half of the Mango team to get ahold of daffy. We got in touch, he patched the bug, and I stayed up for another few hours to prove to him that the bug could allow someone to drain all of the token vaults. I’ve since hacked up a working proof of concept which proves that the bug allows anyone to drain arbitrary amounts of tokens from any vault they want.

I’m not going to make a case for how much the bounty should be, I think it’s up to the DAO and I don’t feel comfortable negotiating for rewards anyways. I just hope the extra context makes it clear that the full scope of expectations wasn’t very clear and that I didn’t personally feel that I was reviewing in the context of the requested code review from a few months ago. But I can certainly see an argument for that as well.

All this being said, I’m just happy that the funds are safe, won’t be upset to not get any reward if it comes to that. But I also would really appreciate one, all the same. And if it’s helpful, I’d like to be more invested in the protocol and would prefer the bug bounty to be paid in MNGO tokens rather than USDC. I’d even suggest that the bug bounty specifies locked MNGO as a reward instead of USDC going forward. It’s a better incentive system in my opinion.

Cheers!

  • Mango Wolverine
11 Likes

I’m my opinion it’s only a prerelease review if it is finished pre-release. I consider our previous payment a prepayment to the bug bounty. It was good decision, because apparently it made Justin look at this stuff on his weekend time off from probably one of the hardest jobs in the world. Luckily he did so before anyone else figured it out, the advance payment might have helped :wink:.

I’d propose we award Justin 3M MNGO minus the prepayment so 2.2M MNGO locked for 5years for the v3 issue. In addition we should pay an adequate fee for the prerelease review of v1, I’d recommend 250k MNGO. So a total of 2.45M MNGO.

We want him to continue looking at our stuff, he is after all one of the best solana developers in the whole world. In case Justin finds more bugs we should again pay him the adequate bug bounty unless it’s a pre-release review.

3 Likes

For the curious, here’s a write up on how the exploit works: Mango v3 signer bug · GitHub
I also created a proof-of-concept which sets up transactions to exploit the bug by creating a spoofed BTC/USDC open orders account and steals USDC funds here: Mango · GitHub

I tested the POC on a local validator which I ran with solana-test-validator and a ton of --clone arguments to copy over state from main net-beta.

4 Likes

Thanks for the write up. Definitely going to keep this in mind when reviewing code in the future.

One thing that comes to mind which could have avoided this would be if mango used PDAs for their serum open orders accounts.

Thanks for the write-up, it’s good to get the additional context and a better understanding where things could go wrong.

As to discovery of the exploit, I think it’s fair to say this is a bug that was found outside the scope of the audit because it was intended for a few months ago, was shelved, and daffy started making arrangements for someone else to complete it. Also the fact that the exploit was found on the live version of the code is important because, for arguments sake, the bug could have been introduced after the audit was completed. In any normal circumstance, you wouldn’t be expecting an auditor to continually review code changes into perpetuity.

Thankfully due to Mango’s generosity and Justin’s diligence, a high severity exploit was found and patched that may not have otherwise been.

Please reward Mango-X so he can coat his coding claws with adamantium and be ready to fight any future Mango-Xploits he may encounter.

2 Likes

The proposal is up for vote here: https://dao-beta.mango.markets/dao/MNGO/proposal/8rRdjAQ6pfaa22STvriE949GCnqDJqM5gcBbwdG85E58

1 Like