Easy-to-Miss Solidity Bugs | Jonathan Mevs & Michael Boyle - Quantstamp

[Music] all right hey everyone uh my name is Michael and I'm John and we are both auditing engineers at Quant stamp uh web 3 security firm So today we're going to go through a little more of a technical talk and we're going to lead you through some easy to miss uh solidity bugs um first of all obviously security is super important I think as some of you guys saw from the last uh presentation we've gone through some of the security concepts of how to develop things from a high level uh we want to dive into some very specific solidity bugs that we can look at um from actually real world audits so everything that we go through today is actually going to be from an audit that we have done uh and then we have simplified the bugs and tried to boil them down into a couple slides that we can share with you guys um obviously in any security sense in the web 3 space especially uh we have lots of money at stake uh reputation at stake and so it's essential to avoid bugs at all stages of the process in order to you know prevent losses and prevent reputation damage um so yeah the goal of this presentation is 100% to get people aware of bugs that are lesser known but still very common and very easy to miss in our security reviews yeah so before we talk about easy to miss solidity bugs uh just to give a little background here like we consider some bugs to be easy to find um and these are bugs that usually you cover in the beginning of your smart contract uh like career uh as soon as you begin an audit there one of the first things you'll do is open Zeppelin's ethernut exercises um which cover a lot of these easy to find bugs we have mentioned here like reentry risks proxy Mis implementations onchain privacy issues signature replay etc those things are pretty welln and standard they've been around um for a while so these bugs are welln and shouldn't really be leading to hacks um in new products uh additionally we consider easy to find bugs anything that can be detected by a static an analysis tool such as Slither so this category of bugs are bugs that we consider easy to miss so during the line by line uh manual audit process of a security audit these are bugs that maybe at first glance they appear correct the logic looks uh correct the logic looks sounds however um due to nuances either in solidity language or the evm or networks themselves uh this can lead to significant issues uh down the line all right so the first issue that we're going to dive into is op code functionality on various evm networks uh a lot of developers especially firsttime developers are going to look at the evm and solidity and they're going to Dive Right In developing based off of the current state of the ethereum main net and unfortunately with how everything is going right now with l2s not every network is 100% evm compatible and so there are op codes and lines of codes that will work in one way on one chain and another way on a different chain and so you have to be completely aware of this kind of situation so a good example here is the self-destruct function uh this is actually being deprecated so I wouldn't really recommend using it anywhere uh but on ethereum mainnet it will no longer destroy your contract it will just return all of The Ether to you however on an L2 this might actually do nothing it the op code will be dead and just revert and so if you were expecting to be able to withdraw all of The Ether from your contract using self-destruct on an L2 and you had it audited and say didn't tell the Auditors that you were expecting to deploy it elsewhere you could run into a situation with locked funds so there's a few different solutions for how you could go about avoiding this problem uh one that we have displayed here is using the chain ID as an indicator to show uh how you want to operate on different evm chains so for example example if the blockchain ID is one you're on ethereum main net you may want to just call self-destruct and get your ether back even if it doesn't destroy the contract maybe on an L 2 instead with a different block ID number you want to call transfer to move your funds out of it uh another way you can do it is to have different versions of your project for different chains I think this complicates the process a little bit more uh and then finally is kind of a uh illusion to one of our papers that we've done recently actually was about a pre-compiler step where you would pre-process your solidity to produce a gas efficient version of it for each chain depending on different op codes that might exist there yeah so this bug is really interesting and uh kind of inspired this talk um this contract is a very simple staking contract where the user can deposit funds however the issue is very subtle here in the fact that the starting stake variable is declared in the memory space of the smart contract and the total stake uh which is actually the smart contract storage itself never gets updated um in this line here that we're pointing to so um as an auditor just really be careful about local variables and where they're existing in the smart contract storage and as a Dev please double check that the contract storage itself is actually being updated um and this is quite easy to miss because it seems like you're taking the user's message. value updating it to starting stake but this total stake variable in the contract stat stuff never actually gets updated so the fix here is pretty straightforward you would usually only use a local variable for intermediary operations like we see here uh calculating the net user stake and then the contract State needs to be directly updated itself so after the stake does take place the total state would be reflected in the contract State all right another uh instance that is kind of the inverse of this one is using storage variables in a function when you shouldn't uh so in this instance we have a staking contract and we're going to say that we have a struct which is used to determine how much ether needs to be withdrawn from the contract uh in this bottom function here we have the process unstake where we're going to take the unstake request and store it in a variable within the function and the bug here is that we use storage instead of memory to store that struct so when we grab the request storing it we're not actually storing it in a memory variable in the function we're just having a pointer that looks at the storage in the contract so on the next line when we delete the storage request we are actually going to get to this third line where we're supposed to send the funds and it's going to be completely zeroed out um so you have to be very careful about where you're where you choose to store things in your contract uh so the obvious solution which may be a bad fix would be to just let's just reorder the lines if we delete the unstaked request after we withdraw then it should be fine right well this actually introduces one of our easyto find uh well-known bugs which is a checks effects interaction uh violation which is introduces a re-entrancy bug so you would need something with this like a re-entrancy guard to actually prevent this from uh causing lost funds a good solution to this would be to just store the uh request in a memory variable so now instead of just pointing to where the values are in storage that we want to withdraw we're now creating a copy of the values putting it in the stack and then we can use that value even after it's been deleted uh from Storage to withdraw The Ether awesome uh so this bug involves uh incomplete struct initialization and we see this come up in our audits often times in uh complex contracts that need to define something like a configuration struct uh that has many different fields so in this contract we have another staken contract and in the Constructor uh the intent of the developer here is clearly to initialize all fields of the stake config to these default values however uh it's a little easy to Mi or easy to detect that uh the stake boost wasn't assigned uh in the stake and did I say easy it's hard to see that sometimes right you can't see that if there's a lot of different fields so what ends up happening is stake boost is going to default to that zero value and as soon as the user Stakes they're actually zeroed out the funds that they've sent in which is clearly no good uh for anybody trying to use your product so um a good fix here uh there's two fixes right so instead of assigning using dot notation uh for the struct itself you can lean on the compiler's safety uh guarantees by using the struct uh Constructor syntax so the solidity comp compiler itself will actually throw an error if you are missing um a field in your Constructor syntax that isn't detected if you were to use dot notation to initialize the fields in your struct so this would be the preferred fix because you do get those security guarantees from the compiler but if you insist or you prefer to use dot notation to assign the fields of your struct a good practice is to compartmentalize your logic into different functions such as something called a nit fig so that devs during code reviews and the development process uh you can look here and make sure that the struct is actually initialized properly and during the audit Auditors would be able to look there and guarantee that the struct was indeed initialized correctly all right so this one plays off of the struct initialization a in a similar manner um by looking at how evm handles default values so in this case we have a pool launching contract we have a pool status of State variable and it's going to hold an enum of type pool status so in this version we have a launched paused and closed are the three states the pool the developer expects the pool to ever be in and then we might also have like a pre-launch phase that we expect users can't interact with the pool yet but we say that it's not launched yet so we don't represent it in the enum because we wouldn't they don't think they need to but since the status was defined as a state variable and then not not set during the Constructor it is actually going to default to the first value in the enum always and so that would mean that right off the bat as soon as this contract is deployed the deposit function would be allowed because the pool status state would be in the launched phase this may not be intended and it can happen in a lot of other different circumstances too where anytime there's an enum and the default value is the first one if that's not an okay or safe state to be in then we need to evaluate adding a new one on top of that so our fix here would be to add a pending State as the first state in the in the uh en know this would make it so that it defaults to the pending State and then needs to be manually changed to one of the launched paused or closed States later great so I I like this issue because um there is no issue and the compiler actually allows this to happen um but there are some things that can really go wrong here in this contract we have a parent contract deposit and a child contract deposit with access control and I'm sure we can see that both of them are incorrectly declaring a private variable account manager but because these variables are declared as private there's actually no um like Collision of this storage as one account manager lives solely within the deposit contract and another lives solely within this child contract deposit with access control so we can see that only the child contract allows the account manager to be assigned and it's only going to assign to the state variable within it itself and this will not be reflected in the parent contract so when the inherited function get account manager does get invoked on the child contract it's actually going to read stale data from the parent contract which would likely just be an undefined zero address value here so this isn't an issue again in the compiler um but it can lead to issues uh down the line as we saw in client code and quite frankly the issue with uh the code that we were auditing here was that deposit this parent contract was never meant to be deployed as a standalone contract on its own so the devs should have just declared it as abstract um to be only inherited and then we could just isolate the storage only into that abstract contract so there's only one account manager ever being referenced uh throughout the system all right so this is one of the more complicated ones that we have here this contract is well it's actually two separate contracts uh parts of a bridge so in the top part we have the origin chain where you'd be sending tokens from one chain to another and the bottom you'd have the destination change chain uh the issue here is that on the origin chain we are allowing users to specify a payload which would allow for arbitrary calls uh to on the destination chain so you might have some say you want to bridge usdc from maintenance net to say zerit L2 you might have something that happens on zerit that you want some action to take place when the funds actually arrive there and so we have want them to be able to specify what actions they would like to take but the problem here is that this is just a little too free um from the user standpoint so the risk here is that we have stalled transactions lost transactions um when you're focusing on something that's going going from one chain to another you lose the atomic guarantees of operating within a single chain and so now we need to focus on thing problems that could occur uh if say one transaction doesn't execute on the second chain can it be replayed uh if it gets to the second chain and fails can it be retried or is it going to fail every time is the user paying enough gas on the starting chain to pay for the operation on the second chain and when you give all these options such as an empty uh payload that the user can specify anything we're very likely to run into issues so as I've kind of been alluding to the auditing tips we anytime you have generic bytes as user input we're probably doing something wrong unless we're working with like a wallet uh we want to confirm that our cross chain execution is in somewhat of a box state so that we can limit the number of operations that the user can actually perform and we want to remember that different evm chains again operate a little bit differently so just because it sounds like good op codes on the origin chain doesn't mean it's actually going to work on the destination chain so once again we want to sanitize inputs we want to restrict and give options for what the user can do instead of allowing for arbitrary execution uh we want to enforce maximum payload sizes so that we don't run into issues where um actually I should go back so we have a tri catch block on the destination chain and this would theoretically handle any erors that we would run into but sorry we activated Siri um this would theoretically handle any erors that we would run into but the one it can't catch is if you run out of gas on the destination chain it's just going to fail and the whole bridging operation won't work so we need a way to work around that and limit operations that are going to run out of gas on the destination chain awesome so those were our main category of lar um easy the Miss solidity bugs but we have some honorable mentions out there so any contracts that support multiple tokens please do not assume that all decimals all those tokens will be the same so perhaps read the contract State for the decimals when um normalizing values uh any nft marketplaces please keep your eye out if it's planning on supporting the Crypt Punk contract the Crypt Punk contract has been around uh before many of these token standards uh became the norm so it operates in a very unique fashion uh additionally if there's a any staking contracts and uh there's needs to be accounting between user stake funds and the reward tokens that are given so if usdc can be staked and rewarded out there needs to be an accounting separation between the users funds that were deposited and the rewards that are to be paid out often provided by the team uh which sort of relates to like the risk of just reading the balance of the contract and then another thing that we've seen is uh if usually this is in the earlier stage of a protocol but if there's any manual price updates that are to be given by the team uh maybe it's like a ratio of Rewards or something like that that is almost always going to open uh up the contract to the risk of front running and back running for optimal gains and maximum extracted value so uh with those before anything like uh withdraw for a certain kind calc reward if that can be impa uh reassigned by the team perhaps prefer an update on those operations instead of waiting for perhaps like a price update once a day yep so um in conclusion even our easy to miss bugs can lead to severe consequences uh lean on both static analysis in-depth audits manual Line review any automatic tooling during the audit uh process itself then just generally Embrace best practices for solidity smart contracts such as proper State updates full initialization do not rely on default values it's even safer to assign the default values explicitly than to just uh assume that they're going to work there and always always sanitize the data especially if it's user provided so stay proactive always read up on crypto Twitter security best practices recent hacks and stay safe out there awesome thank you guys we'll be around to chat after we have about a minute and a half if we have time for questions I don't know might be close any questions thank you looks like we got one can we get him a mic he was raising his hand oh yeah oh yeah so does Quan stamp do security audits for other chains besides uh evm chains yes definitely we've done audits for uh all evm chains salana U we've done stuff for suie we've done stuff for ton yeah we've done cardono we've done just about everything uh including the kind of web 2 node s side of things as well uh is there any talk about using AI to automate some of these audits that you guys do manually uh we do not have a talk on it but we do if you want to come talk to us later we do have some insights on that also oh maybe let's talk after let's talk after down there thank you guys again