Solidity Audit Process: Common Bugs and Best Practices

Auditors must scrutinize Solidity code to prevent bugs from misrepresented accounting and potential attack vectors.

### Common Pitfalls in Solidity Auditing: Identifying Bugs and Improving Practices
* 🔍 Many bugs in Solidity arise from **incorrect accounting practices**, such as misrepresenting values like `message.value`, which can lead to **discrepancies in ether transfers**.
* ⚠ During audits, it's crucial to verify if calculations, like **multiplying variables to determine total ether locked in a contract**, could yield **incorrect results due to oversight**.
* 📜 For aspiring auditors, a comprehensive resource document is available, detailing everything from **basic Solidity coding** to **auditing firm listings**, priced at just **$100** with a **20% discount** for viewers.
* 🛠 A thorough review of project documentation, such as README files, is essential, especially when they **lack clarity**, to ensure **proper setup and understanding of the project structure**.
* 🧪 Utilizing tools like Slither can uncover potential vulnerabilities, such as **reentrancy issues**, which auditors must address to **secure the contract effectively**.
* 💡 Observations about gas optimizations, like using **`bytes` instead of `string`** for static URIs, although minor, can **enhance contract efficiency** and should be noted during the audit process.
* 🤔 It's important to **question ambiguous variables** and **document uncertainties** during the audit, fostering a culture of **thoroughness and clarity** in the reporting process.
* 💻 The audit process often involves iterative reviews, where auditors **refine their findings** and ensure all **potential vulnerabilities are addressed** before finalizing their reports.

hey everyone, um, I had a few people ask me to kind of go through my audit process. Um, so I got an audit in uh, this week, and what I did is I screen recorded um, the entire process, and I'm gonna sort of edit down, edit it down into smaller chunks, kind of where the most notable things are happening in the audit. Um, I've told people in other talks and sort of QA sessions that my audit process is somewhat like what I do is I kind of pull the code down, I look at it from a high level, I put audit comments throughout the code base, um, while I'm trying to understand how the contracts kind of interact with one another, and then I do it. So, I do all that in one pass, and I do another pass where I'm sort of diving into a lot of those high-level comments that I put, and while I'm doing that, I might write a POC or something like that to just either prove to myself or prove to the developer that in fact

um, it is a bug in the code or something like that. So, because the audit process for me is sort of two parts, I decided to break these into two sets of videos. One, uh, where I'm just going over the high-level comments, sort of just walking through the code at a high level and sort of pointing out some of the things that I'm noticing. Um, they may or may not be bugs, I don't know yet, right? And then in the second video, I'll actually dive into a lot of those bugs and actually um, either confirm them or, you know, you can watch me write a POC or something like that for one of those bugs. So, without further ado, let's just dive right in. So, this is the contract we'll be looking at. Um, it's a obviously a private repo. Um, I'm only going to publish these videos um, after the audit report comes out, but you know, generally I'm just sort of pulling the code down into, you know, my own local repository. Um, one of the first things I like to do is

I like to take a look at the README to see if there's anything there about sort of running the project. In this case, it's a hard hat sample project README, which is often the case when you're auditing, right? I mean, the majority of the time, you're not going to get any sort of documentation. This project is no exception, but what it does tell me is like, Okay, this is a hard hat project. And so, I need to get hard hat running, and then I can use that to sort of compile and run the tests. I actually, the reason I was able to book this audit was I had audited a previous set of contracts from this company. It's an NFT marketplace, and so that's what you could see me here. I was just sort of like checking out the previous project to sort of refresh my memory like what Oasis is basically. So, at this point, you know, sort of the first thing I like

to do is like get get the code compiled. Um, there are actually quite a few auditors who actually don't care about this at all, like they don't even care if the code compiles or not. Um, in my case, I think I mentioned in a previous video that I like to use Slither, and um, sort of a prerequisite of Slither running is a compile compilation, like the contracts get compiled so static analysis can be ran on them, and that's kind of the only reason I do that. Um, another good reason though is like I like to use the projects test um, themselves if I'm writing a POC rather than like, you know, having to write my own wiring and stuff. Sort of depends on like how complex the POC is that I'm writing. Um, if it's super complex, I'll just use the project itself, uh, their test libraries. If it's not, then I'll, you know, obviously write it in Foundry or something like that. So at this point, I'm just pulling the contract down, getting Hardhat running, making sure the test, the code compiles, and

then the test run um before diving into my actual audit. So one thing I wanted to point out here is that this is me running the tests, right? And what you'll see is that the tests fail, uh, a few of them. Two of them actually fail, and it's some stupid gas-related thing. Um, I'm running with Ganache, and so like for whatever reason, like the gas price on Ganache or whatever doesn't work with this set of tests. Um, I think I mentioned before, like all I'm trying to do is like get this into a state where I can run Slither. And so even if the tests fail, like as long as it's for, you know, a reasonable reason that they're failing, I don't care, right? So I'm like, I get the test running, enough of them pass that I'm happy, and I move on. So the next thing I like to do is I like to run Slither from the command line. Um, this is just to make sure like if Slither fails for some reason in VS Code.

It's because of the extension and not because of slither itself. Um, in some cases in a project like, you know, so they just won't work at all, and then I don't bother trying to get it to run in VS Code, so it's sort of just like a sanity test so that I can run slither in VS Code without worrying. And while that's running, um, I'll start to sort of just like browse through the contract um, before slither finishes. You can see the output there. Okay, it's great, you know, so they ran fine here. I'm just like sort of updating the uh compiler version in VS Code like, you know, just routine stuff at this point. Okay, at this point like I'm actually starting to look at code. At this point I'm sort of checking this the linter warnings, and one of the winter warnings that caught my eye was um, the visibility is not set for a lot of the variables in the in all the contracts. And so I'm just you're suggesting like, you know, it might be

better to be explicit rather than implicit. Another thing I'm noticing here is like, okay, the total supply variable here is like shadowing one of the superclass total supply variables. So I'm marking this. I end up marking this as a comment in the audit, and then later coming back, checking it again, and saying, okay, like this is actually fine. There's nothing wrong with doing this. Okay, now at this point, I'm actually going to run slither inside of VS Code. Now that I know it works on the command line, I'll run it in VS Code, and now I'm going to get a bunch of like, you know, useful underlining, which I mentioned in previous videos. It's going to take a while for, you know, slither to pop up and like show me all the underlining, but the beauty is I can edit ahead, so I'm going to do that. And you can see here that it's got a yellow underline under the emergency withdrawal, and it's saying that there's a reentrancy here and in

fact there is um there's a modifier on that function right, so it's actually not that big of a deal, but again, it's nice to know another reason why I like so there. It's nice to know that there's a reentrancy there. Okay, so now we're actually like starting to analyze the code, and this is one of my techniques that I like to utilize. I like to go to these library functions first because they're usually small and self-contained um, and it's a good way. It's sort of like a depth-first search approach to the auditing, right? So, I go out to the very leaf nodes of the dependency of the contract um, mostly in order to just like reason about things in isolation. Like in this case, I know okay like transfer eath. I know that there's a call here, and I know that there's potential reentrancy here, right? There's no reentrancy guard on that function, and so it's something I can start sort of keeping in mind every time I see this transfer eth.

function you can see here what when you know something. I do throughout the audit process. I like to google things and like just remind myself what the best practices here, right? So, I'm saying, Okay, they're using call here, and is this still the best, considered the best practice, right? So, I go and I look up, and it's like, Yes, of course it is still best practice, or whatever, right? It's funny, like when you first see this code, it's so small, you think like there can't possibly be anything wrong with it, but I immediately noticed that there is something that's potentially wrong with it. It's not this code specifically, but it's more that like, Okay, whenever you're transferring ether, you have to send message.value, like message.value has to be sent, right? The problem is that they're creating an amount here, they're passing in an amount as the function call, and so I know there might be differences there where like, Let's say user sends message.value and then sends for an amount that's either greater than or less than the

message that's or the value of ether that's actually sent in the transaction, right? So, I'm just making a... There, there's any... There's not a bug in this code, right? But it's something I'm going, Okay, like, where is this... Where is this function actually being called? And let's check that this invariant is held, such that there's not extra ether just laying around in the contract anytime they're trying to do a transfer of ether. And so, like, this sort of illustrates why I like to do this step-for-search approach where I start at the leaf nodes, right? Because now I already have something I can use to dive into the rest of the contract. So, I can basically go, Where is this transfer library held, like, helper used elsewhere? Like, let's go look at that right now and say, 'Is this invariant that I'm expecting to be held, held throughout the code?' So, I'm going to go do that right now, and now I've got this inroad, some sort of like a mental model.

that sort of connects me from this stupid library function that's not really doing much into the actual business logic of the code. Here's a stupid little gas savings finding I had. They're using the string type and um, I think usually like base URI and contract URI variables um, they're not dynamic and so they could save gas if they switch to like a bytes byte array or something like that um, you know frankly these are not like gas savings are not that useful or not that important in an auto report but it's sort of one of those nice to have things like while you're looking at the code if you notice certain things where they can save gas you sort of call them out. Here's a good example um audit comment where I'm like I don't know anything about this variable like I don't know enough about the contracts at this point with hindsight I'm doing a voice over here so with hindsight I now know what this proxy transfer proxy address is for but at the time I didn't know and so I I just

If I see something that smells weird or smells bad, I'll just drop a comment and go. I don't know what this is, but like, you know, look, look, decide to look into it again, right? So this is a good example of something where I'll either, while I'm doing my first pass, I'll answer my own question, or if I don't, I know, okay, in the second pass, I need to spend more time on this variable. So it's okay to just go while you're auditing, just say, I don't know, this looks weird, right? Drop that comment and move on. Here's another minor gas finding throughout the code. They're using uns, and then they're checking like that various all these un's are greater than zero. You can't go less than zero, right? So really what they want to check here is just it's not equal to zero, and that's a gas saving. Here's one that's sort of become a meme in the auditing space. There's like this tension between auditors who want

zero address checks for all the addresses, and then um developers who are trying to save gas or whatever, right. So, I mean, I'm sort of indifferent. I don't really care. In fact, I actually probably on the side of the developers where I don't think it's actually necessary. But I think what happens is the linters and static analysis always call it out, so the auditors feel like obliged. They need to put in the audit report. So, here's probably the first interesting one. Um, so anytime you see dsas used inside of a smart contract, you always want to be checking for signature replayability or signature malleability. Malleability. Um, to be honest, I actually didn't. This is the first pass. I could keep saying right in the first pass. I'm not actually diving into these sort of details, but like almost every time I see ecdsa uses an import in the contract, I'm checking for that. And so, I put a common enemy was like check. Make sure the signatures aren't replayable. I think

actually, in this case, they are replayable. But something I would do in a second pass is like make sure that I'm not wrong about that before I actually put in an audit report. Here's another interesting finding. So, one thing they're trying to do is like limit the number of lots that a buyer can hold. Um, and so they're sort of like, you know, based on the message.sender, they're keeping a counter here and then say if your amount goes above that lot limited number. Um, you know, they have a requirement statement there. The problem is it can be simple to act right. He can create as many addresses as you want on the Ethereum blockchain. And so it's sort of like a useless required signal basically. It can be bypassed trivially. Here's another good comment. Um, it's, it's, there's nothing really concrete about it, but I'm sort of looking at how they're calculating how much money can be withdrawn at any particular time. And I'm just sort of thinking out loud to myself.

Is there is our calculation ever incorrect because I think a large source of bugs in Solidity is like missing the accounting or accounting the same variable in different ways and there they end up being incorrect if that makes sense um earlier in the video I talked about like the case where you know message.value is different than the amount passed into the function right so in that case you're sort of just trying to send ether from one address to another but you're you're sort of misrepresenting the accounting of those two variables right so in this case I'm thinking the same thing I'm like okay in their head they think that if I multiply these two variables together then that will give me the total amount of ether locked in the contract but that might not be correct right and so it's just another thing where I would in the second pass I would spend a lot more time going like can I ever make this number incorrect in some way here's another interesting potential attack vector so um they have the stake royalty function.

which allows you to deposit um ether into the contract and change the per lot royalty based on that deposited amount. And then how many lots have currently been sold out. And so what I'm wondering is I haven't thought any further on this right but while I'm looking at the code I'm wondering is it possible to gain this per lot royalty in some way either by sending an amount and then doing some action immediately after that and then withdrawing the amount so that you're changing that variable or maybe doing something related to the get lot number sold out right. So the idea is like you do something you call stake royalty you do you know recalculate make force the contract to recalculate the per lot royalty and then you do something else and then you know undo all of that right. So that's another potential attack vector that I see often in smart contracts and this might actually be like a really bad case because there's no time waiting on that variable that royalty that per lot royalty value that happens in

that calculation happens in block, right? And so I really think, um, I'm hoping we'll be able to find something associated, some sort of attack vector associated with the calculation of that variable. Because if you look at the function below, it's used to actually calculate how much can be claimed by a user, right? So I really think this is gameable. That said, I know I'm beating a dead horse at this point, but this is only the first pass, so I'm just sort of brainstorming attacks at this point. Here's an example where I mark one of my previous audit comments as okay, right? So that I know I don't need to look into it any further. We mentioned this one before where I was saying, okay, this total supply variable shadowing a superclass variable of the same name. So I just simply, like, once I've done this pass over this contract again, I'm saying, okay, yeah, this is actually fine. But I am dropping a comment here that's probably gonna end up being informational in the report that they

probably should rename the variable to something else just to be safe. Here's an interesting one. So they're um they're using a factory to sort of like deploy a lot of these contracts. In this case like one of the advanced ERC 2721 tokens that they've created um and so it has this problem where like Uniswap has the same problem that they actually solve for which is your factory like if you keep passing in the same parameters um you're gonna get the same address out right. And so I could see an attack vector where like you know an attacker kind of like constructs a factory the same address and create some sort of like an address collision or maybe like pre-calculates the addresses for a valuable ERC 721 or something like this right. So I dropped that as a comment um but what's interesting is like when I dove into the underlying mechanics of how they're creating those addresses they do actually account for that um. It's always funny when you're an auditor and you're like you know you think you

found something, and then the developer has also accounted for it. You makes you think like great minds think alike or something like that, you know. If you like that video and you are interested in learning how to be an auditor yourself, um, I have put together a document of all the resources that I used, um, to learn how to become an auditor myself, starting with, you know, so basic Solidity, learning how to code in Solidity, all the way through to some boot camps, capture the flags, and a list of auditing firms that are hiring auditors right now. So, a link to that document will be in the description of this video. Um, it's only a hundred dollars. I've seen um, other auditing firms charging upwards of twenty-five hundred dollars for the same content. Um, and since you're coming from my YouTube video, my YouTube channel, I'll be giving you 20% off as well. So, enjoy.

My Smart Contract Audit Process (Part 1)

Jackson Kelley

Auditing a Smart Contract: A Two-Part Video Series

The audit process is divided into two sets of videos: one for high-level code review and another for diving into potential bugs.

The auditor emphasizes the importance of sending `message.

value` when transferring ether and notes potential discrepancies with function call amounts.

The auditor uses Slither for static analysis and aims to compile the code to run the project's test libraries for writing proof of concepts (POCs).

Running Tests and Using Slither for Code Audit

The tests for the project fail due to gas-related issues on Ganache, but the author is not concerned as long as some tests pass.

They run Slither from the command line for a sanity check before using it in VS Code.

While waiting for Slither to finish, the author browses through the contract code and notices linter warnings about variable visibility, prompting them to investigate the function calls and ensure the invariant of ether transfer is maintained.

The total supply variable is shadowing a superclass variable, which was initially marked as a concern but later deemed fine.

Running Slither in VS Code provides useful underlining and highlights potential issues, such as reentrancy in the emergency withdrawal function.

The audit process involves pulling the code down, making high-level comments, and then doing a second pass to dive deeper into those comments and potentially writing a proof of concept.

The code uses string types for URIs, which could be more efficient with a byte array.

There's a potential attack vector in the contract deployment factory, but the underlying mechanics account for address collisions.

Minor gas savings can be achieved by checking for inequality instead of greater than zero.

The code is trying to limit the number of lots a buyer can hold, but this can be easily bypassed by creating multiple addresses on the Ethereum blockchain.

The function for withdrawing money has a potential reentrancy issue, as there is no guard in place.

The auditor is focusing on library functions first for a depth-first search approach, while also checking for signature replayability and malleability in the use of ECDSA.

Common Bugs in Solidity and Audit Insights

Incorrect accounting of variables in Solidity can lead to bugs, such as misrepresenting the amount of ether being transferred.

The speaker emphasizes the importance of thoroughly checking for potential discrepancies in variable calculations.

They offer a document with resources for learning to become an auditor, including coding in Solidity and boot camps, at a discounted price.