Smart Contract Auditing: A Practical Security Review Guide

[00:00:09]and i should be live now everyone so just confirmed i am live uh welcome to my first stream uh so i hope there won't be many technical problems but uh let's get started so on today's agenda i will be talking about my approach to audits and then we'll be reviewing some smart contracts live i asked on twitter for people to submit some smart contracts and i've selected some of those and created a list here these are also mentioned in the description of the live stream if you want to look at the source codes yourself you can look here if we have time i have some extras here but um i honestly i don't think we will be able to get to the extras in this stream perhaps for some other day um so before getting started just about myself um so for those who don't know me i'm a security researcher and ethereum developer i've been in the blockchain space for about five years now and uh like i've i started with bitcoin and monero moved to ethereum right now i'm helping sushi swap build their next gen amm uh named trident and that is coming soon so now about this stream the target audience is basically someone who has basic understanding of ethereum and solidity um so for those who are new to ethereum of solidarity these are some of the resources i recommend i have this list in the description as well um so the ethereum book is basically like one of my favorites one of my favorite resource for the newcomers then we have crypto zombies so this ethereum book will give you the basics of ethereum and some introduction to solidity um so definitely start with this then if you decide to continue on with solidity then i think crypto zombies is quite fun um it is an interactive game where you can learn solidity by completing challenges similarly we have solidity by example this i believe was inspired by rust by example or at least i saw the rust version first but this is another great resource to learn solidity once you have done the basics with these two things these three things then i would suggest that you go over the solidities docs just like glance over each and every page of these dogs these can be a bit crude but i can assure you that these dogs have these dogs have some hidden golds inside them some gold nuggets which are like great pieces of advice from the solidity authors themselves you can also look at ethereum orgs website they have a good collection of resources and you can pick any of the resource they have mentioned all of them are of high quality so once you have learned solidity and you want to get into some security type aspects or even if you just want to do advanced stuff this ctf by folks from open zeppelin is i think awesome i really really really like this um so uh go here uh try these challenges if you can so uh this is basically a ctf where you have different challenges and you have to basically hack the smart contract to complete the challenge and these are quite fun and they tell you some like common vulnerabilities in the smart contracts and that you should be aware of and stuff like that so i definitely recommend everyone to try this out but probably best to do it after you have done the basics of solidity then uh i think these are the resources i recommend i will just dive into the auditing approach now i want to cover a lot of content today so um if i'm going a bit fast apologies for that actually let me just open the comments in my mobile so that i can follow if there is anything that is being posted in the comments yeah i have a very jank uh setup for streaming first time doing this so yeah okay i have the comments in front of my me know um so yeah the auditing approach i have some bullet points here uh that i will expand on let me enable word wrapping so uh the approach that i take to audits i recommend like not only auditors follow sort of this checklist or like this approach but i think this can be helpful just for like developers as well who are trying to think uh of their project as like from a securities point of view and trying to fill any uh holes or gaps that might be present so whenever i get a request to audit a project the first thing i do is like whenever i'm starting an audit the first thing i'll do is go over all the non-code resources that the developers have provided me so this will include any documentation white papers marketing material a blog post and anything like that so um stuff that will give you an idea of what the project is meant to do at this stage i do not want to look at these smart contracts because i first try to create an image of what they should look like and then i'll compare if the image matches the matches my description if you like i feel like if you delve directly into smart contracts then oftentimes you just focus on what the smart contract is doing and not on what the smart contract is supposed to do um so this is one thing i really like um just follow all the known non-code resource first and then del into the smart contract so once i've gotten an idea of what the project is i'll delve into the smart contract so the first thing i do when i look at new smart contracts is to just glance over them briefly and to understand the architecture that they are using at this point i will not be reading every line uh like every source code um if a contract says that it is an erc20 token for now i'll just believe that it is a standard erc20 token and move on at this stage i just want to get an overview of the architecture of the smart contract i'll also use tools like surya from consensus to plot some dependency graphs and help me better understand the architecture and how dif how the different contracts uh talk to each other once i have this brief idea of the architecture and i know where to look for something so like if i want to see where the erc20 source code is at this stage i should be able to tell where that actually is in the repository this stage is not really very important for single or small contracts um because then you can like just skip this and get to the second stage or the next step basically in the next step uh i try to create a thread model for the protocol so um if let's say let's talk about sushi swap their decks in that you will say the actors one one actor will be the liquidity provider who is providing liquidity and the other main actor will be the user who does the swaps so um i have like two main actors uh liquidity provider and person who is swapping assets then you can break these further into like smart contracts who are responsible for these actions like the pool router and stuff like that but in the broad overview there are two actors based on that information i'll try to think uh from there from those actors point of view what they will want in this protocol um what can be some like potential exploits that can be present in this protocol i will try to list all the theoretical attack vectors which will include your common pitfalls um so common pitfalls used to include like missing arithmetic checks in solidity so before solidity 0.8 by default solidity did not cause a revert on overflow or something like that so people were like this was a common pitfall that people had a variable that can be easily overflowed or underflowed in solid it is 0.8 all variables are by default strictly checked for overflows and under underflow so this is not as common of a pitfall but yet still is then uh i'll also compare my like threat model and the architecture that i've created to shortlist any like past exploits uh techniques so um past exploit techniques like reentrancy your oracle money manipulation taking flash loans and manipulating the pool balances um stuff like this so basically whatever has already happened in past and we have hopefully learned from those mistakes i'll create a list of those which may be applicable to this application and at a later date i'll verify that these vulnerabilities are not present so increase the font a bit hopefully this is better by the way this content is also present in the description so um if the font is still too small you can read the description uh so i was um at here so yeah at this stage i know the threat model i know the architecture of the smart contracts i know and i know the list of potential exploits that i could have thought of then i'll start actually reviewing the smart contracts so whenever i start reviewing these smart contracts the first thing i do is like search for all places where value is being transferred so this will be like erc20 transfers ether transfers solidity as a transfer function um your erc20 transfer from solidity's send function solidities call function delegate call function and self-destruct these are the most common ways to transfer values i will look for each and every instance of these functions in the smart contract and then i will walk backward from that call and see like how that call is being triggered and uh if this call is secure and it can't be like manipulated or like no unauthorized people can trigger this call and stuff like this so uh walking backwards is especially helpful in large code bases where you have like thousands and thousands or lines of codes they're using this trick you can basically like review the most critical functionality within a few minutes or hours for smaller contracts this is not as helpful because you can in that case just do a line by line review within a similar time frame but i still like to do the most critical reviews first before i get into the line by line review so when i do the line by line review i'm looking for two main things one is your logic bug so um anything which although is not really a security concern but is a logic box so the smart contract is not behaving in a way that it is supposed to so an example of this will be so if i read that in the white paper and everywhere people are saying that um the project should charge a 10 fee on every transfer or something like that but now you look at the contract code and you see that the project is actually charging uh 25 fee rather than 10 percent on every transfer or it is not charging a fee at all so in isolation those issues are not really a security concern but given that we have the context from the first point that the project should have been charging 10 percent fee uh we can conclude here that this is a bug and should be pointed out in the audit report i know not all auditors focus on logic bugs and honestly logic bugs are not my priority either but since i'm doing a line by a line by line review anyways i just feel like i uh go ahead and keep matching everything to the specs um it the main advantage of doing this matching is that you then don't end up making incorrect assumptions about a project so um you might in like in a normal project you might see something and you might just assume that it is how it is supposed to work but for the project you are reviewing it might actually be a security flaw so it always best to match resources the second thing i check when doing the line by line review is for the list of exploits that we are earlier created so um i make sure that none of those exploits are actually applicable on these smart contracts and these contracts are secure from those while i'm uh checking these smart contracts for existing exploits i also try to brainstorm new uh like new styles of attacks or something like that and i think that stage is what takes the longest in my audits um so i usually spend like a day on every smart contract just brainstorming different ways to um like attack this or something if i set this parameter to this value or if i follow this path of like function calling or stuff like that so um i just try to brainstorm different approaches to find a flaw in the smart contract it is your like typical pen testing um i'm just trying to break the security um i don't know what exactly will break i'm just like throwing mud at it and hoping that something will stick so at this stage i might be trying like dozens of proof of concepts and usually only a couple of those will actually give positive results positive um in the sense that exploits that uh like vulnerabilities in the smart contracts so in well-written smart contracts often you will not find anything from brainstorming but in like hastily written smart contracts you can find uh dozens of vulnerabilities uh honestly the worst or best i've done is 23 critical vulnerabilities within the single project um so that sort of code basically needs to be re written from scratch um so yeah then once you have done all the brainstorming you have found out whatever you could then this stage is over at this stage almost all of your review is complete at least the first pass is complete but i like to do another pass from the perspective of individual actors so um in this case um in sushi swaps example i said the actors are lp provider and the person who is swapping the assets so i'll try to get into the mindset of those users and try to do what they are doing so these will uh basically be i will basically be emulating the most commonly used use case of this project and um like i will so a liquidity provider is supposed to add liquidity remove liquidity i will try to do those two i will try to call those two functions in various combinations and permutations uh whatever i think makes sense um then from the swap perspective i will uh try to swap some assets do some multi-hop swaps and stuff like that so this just like ensures that you are actually following the path that a normal user will follow like oftentimes you'll find when people are testing smart contracts especially the testers they will try all the weird edge cases but they will forget to try the most common use case so this just gives you like another uh review so this is like my second pass over the smart contracts but from the perspective of the normal users just to make sure that the normal use case works so once i've done this pass i will like accumulate all my findings share these with the developers and they will get started on any fixes if i find anything serious before this step then i usually just share the findings as soon as possible so that they can start working on a fix or clarify anything if i'm missing obviously while i'm doing all this review i like to maintain an open communication channel between the developers because oftentimes you will see something that you you may not be sure about like what this thing should do in those cases it is good to talk to the developers directly to know their intent the like if you if the developers create well-written documentation of before submitting the contracts then i think the requirement for that communication like decreases a bit but they still is best to have a direct line of communication after all of my manual reviews i also run some automated tools um the best tool right now in my opinion is slider from trail of bits i think it covers most of the resources resources and is free consensus has their mythics or material um one is like open source one is their service it is also quite good but i personally like just prefer slither from like personal experiences but if definitely you can also check out cds offering finally i'll glance over the test cases and the code coverage um and like add comments there if i see anything wrong um this step is just really for uh like the ending conclusions and stuff doesn't really affect my manual review oftentimes like especially when i'm brainstorming approaches i will write some test cases myself to verify my findings so um oftentimes what happens uh is like i've already covered a vast majority of code coverage in my like pocs and stuff so uh i don't really um depend on the developers test cases but this having good test cases is a good signal that the code was written well and it was thought out nicely and yeah that basically concludes my auditing approach and uh once i'm done with all of these things i wait for the developers to push any fixes or do any clarifications answer any questions once the fixes are ready are ready i do another round of review of those fixes that second round is honestly not as comprehensive as the first round um i don't like spend much time on it but i will just verify that the immediate vulnerability is fixed so um if like i found only a few smaller issues then that is fine i can just verify them in my second review but with some projects i've seen cases where like i found over a dozen critical vulnerabilities and that really requires a rewrite of the project in that case um although i will do a short review of your fixes i really recommend developers to just get another order done with me or with someone else but it is practically not possible to review a project again in a short time span when basically everything has changed in the project so that was it i'll take a short break and look at the chat before i do before i start the live review process i think we can continue with the live review and this is going to be the most interesting aspect of the stream in my opinion um so i've briefly looked at these smart contracts but i haven't really spent much time on these um these are the ones that i think are small enough to review in this call these ones i feel like uh are a bit too large to really cover on a stream so i've put them in extras folders if you have a if you have time later we can come back to these uh but otherwise let's get started so um this contract uh bridge deposit was submitted in a tweet to me um so i think this is so i suppose this is a contract for a multi-chain bridge people can deposit tokens here i know i see the owner can withdraw the balance they destroy the owner can also destroy the smart contract and people can like send funds to it using the receive function i'm just taking like looking at the broad architecture of the smart contract right now we have functions that like set the owner and do some config parameters which are only callable by the owner that's all gold these are like view functions i usually just ignore when i'm doing short reviews i will look at these when i'm doing a full audit but for short reviews i'll just ignore the view and pure functions especially when you are dealing with solidity like newer versions of solidarity in 0.4 this view function wasn't uh like mandated by the smart contract so you could just make like non-static calls to it as well but uh from like from newer versions of solidity if you call a view function solidarity will make a static call to it rather than a regular call although these can still be called via normal calls but um usually like if you're calling them from solidity they will be called by starticon and what that means is that if this function tries to change any state then uh a panic will be caused a revert will happen and everything in the transaction will get rolled back so i don't really need to focus on these much i can just assume for now that these don't modify the state these are just helper functions we have some modifiers and some events so yeah i think i have a broad idea of what this contract is trying to do so here people deposit some funds this just emits an event um so um i suppose now an off chain application is supposed to uh like catch this event and do something of this data maybe like bridge the tokens on a different blockchain or something like that um and then basically that's it to this smart contract these functions for withdrawing and destroying i'm not sure what their use cases i suppose these are present for emergencies only now if i had like documentation for from the div i could have been sure what these are meant to be but for now i will assume that these are just for emergency usage because um like what this contract basically is doing is you are giving all your ether to this owner and you should only do this if you trust this owner um if like to this project i will recommend that you make this owner a multi-sig and a multisig with a time lock so um like no single key should have access to this and before this this like owner can execute any action there should be a delay of at least one day in my opinion um so that like people can at least know what's going on but assuming that people are just transferring ether here without anything in return i assume your users already trust you um i have no idea where that trust comes from but the user here is just trusting the owner to like keep their bargain whatever that might be so um let's also delve into these two modifiers first actually let's look at all three so is lower than max deposit just verifies that message value is lower is owner just verifies owner can receive just verifies this is true and this ensures that the current or like max balance of the contract is less than this i will check if this variable can be modified elsewhere this seems like an important variable this is set in constructor that can be called only once that's good and it can only be called by is owner afterwards okay uh we'll also verify that the owner variable can't be modified by anyone owner is set in the constructor and after that i think we had a set on a function search didn't catch this for some reason oh it did it so um yeah set owner function again can only be called by the owner so the owner can't transfer the ownership to anyone else so i think this um like this is as far as i'd go for this contract because you can clearly see the contract does not give you anything in return all of these secure like there is nothing wrong here as long as the owner is like reliable and trustworthy the only point i will point out in this smart contract is before you interact with a smart contract please make sure that the owner is someone you trust you are essentially transferring your money to this owner in this smart contract okay i'll move on to the next contract let me look at the comments briefly for first so for the fuzzing approach um saurabh i do use fuzzing but not for everything so uh like some people like at sushi especially we are doing formal verification with help of satoru so their formal verifier actually does also does fuzzing in case it is unable to verify something formally um so we do some fuzzing but we don't do fuzzing for literally everything in our contracts that i think is a bit too kind time consuming maybe like definitely i can do that while doing audits because fuzzing a complex smart contract can take months and months of continuous fuzzing but i do recommend people to like developers themselves to write test cases or do fuzzing themselves for the most critical functions like we do with help of satora at sushi swap and scar you're right some fixes can open up contracts to another or like other vulnerabilities and this is why i recommend people that if you are changing your smart contract heavily in a fix then you go for a fresh audit your current audit basically becomes like ineligible um or void at that moment but if the fixes are small like just changing one percentage to another then as a reviewer i'm usually able to track all the effects that that change is doing and i'm able to verify that it is fine which extensions do i use in vs code let me just open it here um so i have a dark theme auto close tag yeah i used to write html once although i have not used this in a while auto comments um auto rename tag aws bracket play pair colorization i think it this one actually is quite nice um cc plus plus c make um code lldb rush creates a decompiler docker ethobor shout out to tintin by the way from consensus he has some great plugins for vs code get merger github co-pilot um get a co-pilot actually saves a decent chunk of time like it it is basically a more like advanced version of autofill um it is usually incorrect when it tries to predict a long codes like this or something but just filling out the name like if i enter s it will recommend me string so even that saves a bit of time kit lens go graphics so this can this one is another nice plugin from tintin you can use this to plot the dependencies between different smart contracts jupiter json a bunch of markdown stuff um some diff basis on path intelligence um honestly like as you can see i have a bunch of stuff installed i'm not even sure if i use everything anymore but i do have it printer is also nice python stuff rust analyzer so another shout out to rust analyzer if anyone uh uses rust this i think is much much much better than rls like it is uh like a modern alternative front end to rls and i think it's quite great um so i have a solidity plug-in installed and i have solidity metrics um this i this is another great plug-in from tintin um this i used to like create some basic data about smart contracts i think this highlighting also comes from this plugin although this has this does not always work on all projects especially hard at projects but otherwise this is um i think another nice project actually this solidity visualizer here this is the one that um gives you all that highlighting um this like underlying inning and stuff um i think the solidity visualizer plugin does start another great plugin from tintin so basically you should check all his plugins out um systemd stuff typescript wiper web assembly and yeah i think that's it so um about timelock first of all about re-entrance reinterrency is yes it is one of the like uh previous attack vectors that i check in every smart contract i'm in this contract specifically i'm not worried about it because only the owner can actually call it so if the owner is like the owner does not need to re-enter in any way they already have control over the funds uh and this receive function is not giving anything back to the user so again re-entrance is not a problem here um although there's nothing that you can re-enter with here anyway um so yeah time lock um so time lock uh actually i do have a time-lapse contract that i will be reviewing later today but time lock is just a guard on a multisig where you say that um if a multisig has to execute an action usually a multisec but it can be anyone um if it has to execute an action then it must submit its intent to execute an action like at least one day or 12 hours or some time period before it can actually execute this action so that way whoever is like uh using that project can tell in advance that this thing is going to change in this project so uh maybe uh they will want to exit the project or something like that so here um although like time lock won't really help in this case because there's no way for the user to get back their money um but if there was a time lock and let's say the time lock was one day now if the owner wanted to withdraw balance from the smart contract they will have to create a transaction on the blockchain and in that transaction they have to show that they intend to call this withdrawal balance function now everyone has 24 hours head start they know that the owner is going to call this withdraw balance function so um if this is something malicious or if this changes their opinion about the project they can now exit the project and the owner will only be able to call this after 24 hours have passed um so this is the ideology bit behind time locks and yeah i do have an race contract under review like in the to-do list today i will go over that so um about multi-sigs i i don't know if this doesn't seem like this time lock function has a multisig but from multisec i mean like in ethereum the multisets are not like btc where you can do like combine signatures aggregate signatures in ethereum multisigs are implemented as smart contracts so that means that they are just an address they are nothing more than the more than an address for this contract this contract does not care if that is an end user or if it's a gnosis multisig or something like that um it's just recommended that you set up a gnosis multisig or some other trusted multisig and set that address as the owner here if this was a smart contract that was live i would have gone to the blockchain and verified that the owner is actually a smart contract and it is a legit the owner is actually a multisig and it is a legit multi-sip a multisig basically is uh is a smart contract in ethereum where uh to execute any transaction at least n signers must approve that transaction uh usually if it's an end user the end user just signs a transaction in your meta mask in your ledger or whatever and that action just happens on the blockchain but in case of a multisig uh usually multisigs are n of m so there are uh like m of n there are n uh number of signers and at least m of those signers must sign a transaction for this to execute so if there is a two by three multisig then at least two individual signers must sign a transaction for that to execute this means that if one of the signer loses access to their keys or their key gets compromised um their system is compromised or something like that the hacker will still not be able to like uh do fraudulent transactions from that multiset because they need a control of at least two signers in this case so this just adds another layer of security um obviously like in a real project you will also want to verify that the signers are different people in reality uh someone can just create 10 different accounts in their metamask and set them as 10 signers that doesn't really increase the security much so in real projects you do need to be careful about all of those quirks okay uh so complex smart so uh like i actually have uh learned a bit of calculus and discrete mathematics in college so i am able to look at uh like most of the stuff but when it comes to like advanced like super advanced formulas and mathematics uh i do like uh tell the like developers or whatever that this is uh beyond what i can comprehend or what i can audit and you should like ask someone else to verify um obviously uh your like white paper and uh material like documentation comes handy in this case because even if i can't verify that the math is doing what it is like intended to do i can at least verify that the math is doing uh what the documentation says that it should be doing um although like it might not make financial sense or whatever but at least i can um verify those things so characteristics of hackable smart contracts um i don't think there are any such characteristics something to catch on will be um like no test cases or this might be like a bit picky but if you see a smart contract that is not indented indented properly um if the code is like this um it just shows that the developer didn't really care about the code and that can be a signal that this contract might be vulnerable but there aren't like like any real characteristics like that okay um i think we'll move on now uh let's get to the options this is the smart contract from andrey um i actually looked at the earlier version of this when he originally released it i was gonna review that on stream but uh just before the stream i noticed he updated it with this new version um i think he added uh nfts and something um so let's review this live i haven't reviewed this one yet again we'll start by just looking at the broad architecture of the contract before that i can give you some like context about what this contract is supposed to do this is a liquidity mining contract so usually when you do liquidity mining you get rewards in like native tokens of the protocol so on sushi you might get uh like this sushi token as a reward um now this sushi token you can do anything with like you can stake it you can sell it you can do anything with that this alternative approach says that instead of giving the token directly you give a call option um to the user so uh what a call option means is that there is a fixed price set of the sushi tokens so of the token so let's um take example let's call our token chart token so let's say this chart token has an option which is which has a strike price or base price of ten dollars this implies that um before the expiry of this option you can like at any moment exercise this option and buy the promised chat token for 10 dollars doesn't matter what the price of chat token is at that point if that if the chat token is trading for five dollars in the open market you will still have to pay ten dollars if you want to exercise this option uh again you are free to not exercise this option um if the strike price is higher than the market then obviously it makes sense that you just forget about this option and buy the token from the market um if you want but the interesting case is when the option is in the money so when the price of this chart token in the market is actually higher than 10 dollars um so this chart token might be trading at twenty dollars um in like after some time so now you see you have an option pending of uh chat token ten dollars but it is actually trading at twenty dollars in the market so you have a good arbitrage opportunity now you can exercise your option buy the chat tokens for ten dollars and then sell those chat tokens for twenty dollars or you can hold those chat tokens if you want but the end of the day you are buying chat tokens at a discount by exercising your call option this is helpful for liquidity mining because this just gives a base value to your lake token um if you are giving rewards in your chat token and you price the um call option at ten dollars this sort of guess gives the token a base value of ten dollars people will not really exercise this option and then dump it for less than ten dollars because it will cost them ten dollars to exercise this option so either people will just not exercise this option which means there is uh not much sale pressure in the market from these rewards or the price of chart token is higher than 10 dollars which was your leg base price and people will now be exercising these options and doing like taking the arbitrage opportunity or like holding the token or whatever in that case you don't really mind because the price of your token is already above what you want or what you expect or whatever um so this just like gives you that protection and stops your token from dwindling dying down in some sense um or just a disclaimer i'm not an expert in finance or markets this is just uh what i like understand from my understanding i might be wrong uh so feel free to correct me in the chat oh and this is not any financial advice or anything these are just for educational purposes and whatever like i understand from these things so yeah here andre tweeted this out just explaining what um the contract does let's move on to the contract itself we see an erc721 this is a common standard for representing nfds and this is just an interface so we'll move on okay um this is another erc which like helps other contracts know what interface that this smart contract supports in practice this didn't really pick up much steam very few contracts implement this but there's no problem in implementing this if andre wants um this is a library that i will just ignore for now i'm not sure if any of these functions are actually used if we see a use later we'll come back here same with this library here c seven two one this is the implementation of the contract um so if i was doing a full audit i will uh at least verify that like i will at least verify the basics of this implementation that this is doing what a token is supposed to do but since we are doing a quick review a relatively quick review i'll just assume that this is a standard implementation and andre hasn't um like inserted any back doors here or anything okay we'll move on seems quite standard till now one thing yeah i'll just move on for now see i'm still like glancing over everything to just like if anything catches my eye i'll stop but everything till now has seemed standard yeah c20 interface correctly interface and this is our main contract finally after 500 lines of code okay so we have about 200 lines of code in this smart contract let's just read what it does so they have hard coded and oracle address i'm actually look at etherscan what ps this is for i think it's a generic uh oracle not for uh specific oracle um this is the uni uh v3 uh oracle helper in some sense i believe uh we'll skip over this for now assume the oracle is honest um duration precision develop okay this uh like immediately tells me that this contract is um like inspired by the master chef contract from sushi swap um this like style of maintaining debt of user reward and the reward separately um it's just that i think was done first in masterchef contract i might be wrong but this tells me that this is that style of contract so uh i'm just like connecting the dots here now i already have uh put in a list of common uh like pitfalls that people fall into when implementing uh contracts like that and i know a few hacks that have have happened on similar contracts in the past um so those vulnerabilities i will be verifying uh later uh let's just keep continuing on with the broad architecture first uh we have the option struct uh here um one thing i'll just say although it's not important from security point of view but one thing you can do is uh pack these fields together boolean is just you and 8 in solidity and a field is you in 256 you joint also implies you in 256 so um expiry i suppose is a timestamp uh in uint32 you can fit timestamp up to um 2032 i think or whatever so you can like use un 32 here as well and if you use un32 the bowl will get packed together and you will now be using one storage slot rather than two which reduces your gas cost um if you want to be safe you can also use going to uint64 um or anything higher than 32 and you went 8 and 64 still get packed together because 64 plus 8 is less than 256 and every slot is 256 bits in solidity this is your balance amount and this is i suppose the strike price um these i think although the amount is in un-256 in erc20s but in reality it will likely be uh you it will fit into unit 112 or going to 128 let's say a more standard number because that is the limit that you can use in sushi v2 pools you can't put in more amount that than that so um i think it's fair to assume you won't be putting more amount than that here either so and strike price as well it's just a price it can say easily fit in joint 128 um so i will recommend packing these two together as well you went 128 plus you went 128 plus you went 64 plus you went uh plus boolean is you went eight now you are using one slot for this one slot for this rather than total four slots you have cut down your gas cost by fifty percent you can actually go one step further now um so you went 128 that's fine for amount for strike you can be a bit uh like you can use less uh like bits for strike price because i don't think like the price will need such a broad interpretation or whatever uh if you use you went like 64 minus 8 will be 56 if you use you and 56 i think you should have sufficient bets to express whatever price you want while still being able to pack all four of these in a single variable so 128 plus 56 plus 64 plus 8 will give you 256 and this means that this whole struct is now just one storage variable you have gone down from four storage reads and write on every read and write to just one storage read and write um although like this doesn't affect security but um it is like a gas optimization that i'd usually usually add as an informational comment in my reviews okay so we are storing an array of options um and next index is unit 8 and we have a mapping of address to balance of the user let me check one thing in the c20 if this this balance off shouldn't be overloaded but i'm just gonna confirm that it's not yeah so this um like erc721 uses older tokens rather than balance so we are good i was just verifying that this is like not oh this variable does not overwrite the erc7 to once variable um although i was like confident fairly confident it does not but whenever you come across something that you may not be like sure about it's always best to refer back just like i did then we have a constructor where we are setting a few parameters um last time reward applicable this is fine reward per token and this again does something similar to what master chef does so if anyone has seen the sushi swaps master chef contract they will recognize this style of coding and is just returning the amount earned by the user these i'll skip so uh yeah deposit functions okay so we have two deposit functions um so this is function overloading the parameters are different if you call the deposit functions without parameters it will just like issue this token um i suppose it does an issue or whatever whatever position it is giving to the user it will give it to this message sender well in this case the recipient is different from the message sender okay so we have a couple more deposit functions amount so if you want a custom amount rather than all of your balance and if you want both of them as custom parameters okay all of these finally call this internal function this updates the state of the message render and takes the tokens from the message center to this address increases the balance of the message center of the two which is the recipient usually the message sender and also increases the total supply it emits a deposit event we have a withdrawal function that as opposed to the reverse of deposit here we have similar four functions and withdraw does indeed reduce the total supply reduce the balance of the message sender updates the message in the state transfers the stake token that you originally logged to the recipient and um it's not doing the report here maybe it transfers the reward in update and yep that's it so the claim function um this one um let's see where it is being called okay so this is just to claim the rewards of the message sender this is interesting so we are getting price of the asset um we are getting the expiry of the asset of the option which is block timestamp plus a hard-coded number um then the oh yeah so for rewards we are basically giving options to the user um so here when the user claims the reward we are just giving them an option we have pushed the option here amount strike price and stuff and minting we are minting message center next index um so this next index represents your option in this options area and we have minted this um i suppose this is the erc721 function which and this like nft is minted to the message sender and we have emitted a created event one thing i will point out here so since the withdrawal and deposit function supports a recipient address one feature to add will be to also support a recipient address here in the get reward function so that like you can mint to a specific address rather than yourself no security concern here but just make sense to have that feature so redeem is approved or so i suppose this means someone is exercising their option yep no other option to exercise so something happened is approved or not sure what i'm clicking interest so on this checks that the token id exists and the owner the spender should be the owner or the spender must be approved by the owner these are your standard erc20 erc721 function or if it's approved for everyone um this should never be the case in this but yeah so this just verifies permission that is good this reads the option from storage this verifies that the expiry is greater than the timestamp so this means that the option has not expired yet and we are also checking that the option has not been exercised yes exercised yet so once we have verified that um we do a transfer from from message standard to treasury buy with uh yeah so when you exercise an option as i said you need to buy the token so you are buying chat token um let's say the buy with currencies is usdc um so you will be transferring the uh like strike price of that option which is like the price you uh that was fixed earlier that you will be buying the token for um to the treasury um and this does that safe transfer does basically like it transfers the reward to the user so this is now transferring the chat token to the message sender and this is the amount that it is transferring um so it makes sense you pay for the token uh you get back the token after that we are setting the exercise variable to true and options okay so we are this is unnecessary so um the reason i say that is you see here it this opt variable is already a storage pointer so this means that anytime you read this it will be read from the storage anytime you modify this it will be written directly to the storage um you don't need this line at all i'm not 100 sure if solidity does optimize this away or not but in the most naive case this will cause the solidity compiler uh to go to the storage pointer read everything from that storage and then put the same thing back into the same storage literally so you're reading the same content putting the same content back here which is just waste of gas on this line doesn't do anything if this was declared memory rather than storage then you will need this line because memory means that rather than this being a pointer to the underlying storage you are reading the storage right now and saving all of the data in the memory so whenever you like read this variable later you will be reading directly from the memory rather than from storage you will be saving gas and whenever you write to this variable if this was memory you will be writing to your variable in the memory you will not be modifying the storage in that case so in that case you would have needed this line to actually save the changes in storage but in this case this line is not needed get rewards um this is a function just to claim the rewards this i think is the most important modifier in this contract we'll come to this in a bit exit allows you to withdraw um does the interlink withdraw call update yes it does so um it will uh this just combines your get reward and withdraw function you like this would be the case when you want to exit with the reward i think like one comment i'll add here since that function is missing there should also be an emergency exit function that will allow people to withdraw their deposit without getting any reward so um like if um someone like for some for any reason if there was bug in uh well it's not really relevant here because we are only issuing options when minting options so this should always succeed but in your traditional mastership contracts if you were just giving out rewards directly you would want an emergency exit function which people can call to exit their positions if the contract has run out of funds to er give away in our case the contract will never run out of funds because it's not giving away any funds as rewards it is giving options which it can which it can mint from thin air claiming or redeeming these options will fail if the contract is like out of resources but at this stage your or your original collateral is already safe okay so what does notify do this is weird this function is transferring like reward token from message sender to this address for this amount um okay so yeah i think i know now so this the caller will first of all need to approve this smart contract to transfer the functions uh transfer the tokens and uh i suppose this function is to like fill the contract with funds that it can like give away when people redeem their options so if you were the treasury of the protocol you will approve this contract and then in future if you want to top it up or top it off with more tokens you will just call this notify function with the amount and the contract will take that much amount from your balance i look at the rest of the code to ensure my assumptions are correct it's just updating some reward rate stuff yeah so whenever you transfer the tokens it does change your reward rate because now uh although the time has stayed constant the uh funds available to decembers has changed have changed so now you have more funds available to uh like to december so you have a higher reward rate um but this seems problematic drink some water so first of all the problem i see here is that there is no check who can call this function so anyone can call the notify function and trigger this clause which will change a bunch of variables now we need to verify that these variables like changing this very these variables can cause a harm or not from this i'm seeing it is direct um so this can only happen when the period finish has already happened so the original um like contract has like original options offering has been completed only then you can call this function only then you can enter this else if clause and in this case you will set the reward rate directly to the amount divided by duration last update time will be up uh increased and or like set to the current time and period finish will be updated so i don't think this causes any loss of funds at least this if statement but i can see a potential ddos take attack vector here um if the period actually did finish and now the dao is creating a new lm mining program and is starting a new options offering they probably will call this function um so that this can be triggered but someone can front run their transaction um and like call this function themselves and approve this contract to transfer like one way of tokens and so they can amount they can put in the amount as one this will succeed this case is now true this will succeed the reward rate will be set to 1 divided by something which will be 0 because solidity does not deal with float points so the reward rate now will be 0 the contract will not be giving away rewards the last update time will be set to this and the period finish time will be set to period blocked or timestamp plus duration so this means that you will no longer be able to enter this if clause um we'll see if the else clause can like handle that or does it make it uh like problematic or not but by front running the call you are making the doubt to not be able to enter into this if clause in this else clause we see they are calculating remaining which is the period finish minus block timestamp this can be zero this can be one this can be anything leftover tokens um are the tokens that are left uh from like from giving away the reward already i see a very very small so two things i've already noticed one is that the ddos vector is not very uh like problematic because um the else statement also does basically what the if statement does with just extra checks so even if someone front run you um there isn't much of a problem but there is like some well there's no problem here either the leftover is calculated if the rim if you call the function in the same contract then the remaining leftover will actually be the remaining will be zero oh no remaining will be period minus block times time so the remaining will be duration and then the leftover will be duration multiplied by reward rate which we see from here duration multiplied by reward rate will give you the amount so now here you have the leftover is equal to the amount that the person deposited and if we call this function sometime in the future when the mining has already started but not ended then also we'll get the proper leftover amount here reward rate will be updated um plus leftover by duration last update time is this period finish is this this is good um so we are just updating the reward rate of this contract in this function okay now the update modified this was being called everywhere this is the most important modifier reward per token um i guess this just gets your reward rate we look at this now if total supply zero returns distort one if it starts um otherwise the saved reward is fine but to that we'll add last time reward rate applicable yeah so this just gives you the timestamp of where like it if the period has finished the then the rewards should finish at the finish date if the period is still ongoing then the reward should continue on and the current time should be used uh to like get the last uh eligible date basically we do that we subtract the last update time so um if we are doing this then that means uh like since we changed the reward rate on this time um that change is already included or should be included in the reward per token stored variable so we should be doing all new calculations based on the new reward rate and the new last update time we are just doing multiplication with precision dividing by total supply all of this is good we are just um returning the reward per token right now this is like the value of one token deposited that's good um this has been updated this is the storage variable last update time um this has also been updated that is also good if the account is not address zero then we should update the reward details of the user um so user reward per token paid okay so first we are setting the rewards already accumulated by the user um using the earned function this i guess my right click is not working properly um anyway so the earn function returns your balance multiplied very vote by token subtracting to user date precision yeah so this just returns the total reward that user has earned till now yeah so this portion calculates the new reward that the user should be given and then we add the previous reward to this so this gives your cumulative reward for the user um so this is one place where this contract deviates from the master shift in masterchef you don't store the reward separately you just use the debt and calculate rewards on the fly but here you are changing rewards um because you can change the reward rate in the future okay so we calculate the user rewards and we store the current um reward rate per token which is later used in this earn function for the user that's all good and i just verified that even you call notify that is when you are changing this reward rate you are also making sure that you update the reward per token stored before these changes are executed so that you have covered the current reward calculations based on the current reward rate i think everything's cool till now or we'll so at this moment um from my brief overview i can say like assuming that these wave transfer functions are safe actually safe then we then i don't see any major issues with this smart contract apart from some gas optimizations that i've already mentioned but otherwise this does seem like what like it is doing what it is supposed to do i'll also take a look at okay so this token is not erc20 right this is this balance of i just want to make sure that this balance off can't be modified by anything really and this is just a view function this can't modify it deposit obviously adds to the balance withdrawal sub is reading right now the subtracts from the balance and exit takes out all the balance okay i see one potential issue okay so uh i'm just gathering my thoughts here this might be a critical issue um but um so the lik again this uh method is like one of um like common pitfalls of developing um masterchef like contract so i've seen this kinds of errors before and that is how like this is just like one line of thinking i'm approaching right now at this moment i don't have confirmation that this is an issue but from the stuff i'm seeing here this is a potential attack vector that can be an issue in this type of smart contract so let's uh verify that live i'll first briefly describe what i'm thinking um so as you can see this smart contract mainly like heavily relies on this update modifier this date of the user this reward per token stored this signifies that the portion of rewards that should not be paid out to the user so these rewards have either been already paid out or like the user entered after they were eligible for these rewards um so this is a very important variable in these types of smart contracts and this is update this modifier is called on every deposit withdrawal notify everything that's all good but the problem potential problem i'm seeing is that we have functions like um this deposit where the recipient of the this like token of this like balance can actually be someone other than the message sender so you will see here the parameters are being updated for the message sender but we are actually giving the tokens to the to address recipient which can be someone else which means here i am not seeing any place where we have updated the parameters for this recipient this means that this recipient still has their leg variable this important variable initialize initialized as 0 rather than this being the current value of the reward per token um so now that we have established that that this variable is still zero in the like receiving address now uh the recipient has some balance you can see the balance of of two has increased but their like date has not been increased now what the user can do is from this two address call the withdraw function so this withdrawal goes here this withdrawal goes here this withdrawal goes here and this updates the message sender that's good we are calling for from the recipient this time um so we'll again follow this update modifier yeah so now we are entering the update modifier which the address of the user of the recipient address which currently has the reward per token paid set as zero um so we see that this calculates the latest reward per token which is a global variable that's all good we want that it calculates the last time reward uh apple last time reward applicable so whenever was the last update time this is another global variable that's all good we are happy then it enters this if case and it calculates the rewards that this user should get this is where i think the problem lies um you will see we know the balance of the recipient is a high number whatever we mint it that's good this variable is a global variable which is a high number that's good this should have represented this user reward per token paid should have represented um the starting reward per token for the user so this should have represented the value of a token when the user entered so this value should not be paid out to the recipient that is why we are reducing this value from reward per token this basically represents the amount that the user has like either already claimed or they are not eligible at all for a normal user if they had called deposit function with their message sender and message sender as a recipient this would have been a very high number almost same as the reward but token exactly same within the same block but later this one would have slowly increased while this would have remained constant so the resultant of this would have been a small number and the final result would have been also a small number balance multiplied by a small number gives you a small number um precision you can ignore we also this addition of current rewards of the user this also you can ignore um the current rewards are zero for our user but in our use case when the when we are using the recipient of accounts this variable is zero and i think this is a critical vulnerability now uh you are paying the user from the first date that the program started rather than from like rather than paying the user from the date they entered so if someone enters um the liquidity mining on day 30 let's see after one month uh and they used this trick they will still get rewards of like whenever they claim their reward let's say they reward they claim the reward on the same block itself like they do a deposit transaction and they immediately withdraw their balance um ideally um this these two variables should have been same so they get zero rewards but due to the trick we discovered this variable is zero so the reward they are getting is the full reward from day zero uh up till now and that is how they can get more rewards than they deserve and i think this is um this seems like a critical vulnerability to me um i might be missing something obviously like if i was doing a full audit i would create a poc at this point and talk to the dev andre in this case to verify my findings and to see like if i missed anything or whatever but since this is just a quick review i think i will just end it here uh seeing that i think this is the scenes are putting this seems like a potential critical vulnerability where you call a deposit with a recipient which is a new address which hasn't interacted with this uh like contract before and then you immediately withdraw the balance from that recipient and you get higher rewards than you were supposed to okay i'm gonna take a small break now uh break there i'll check the chat if there is anything interesting there so on the selector comment i suppose you mean yeah these ones so yes these uh will these are selectors in solidity um like these can be selectors but let me verify what they are being used for here oh yeah these are being used to register the interface uh my right click is done for so we'll fall back here so these are not doesn't seem like these are solidity selectors although solidity selectors are also bytes for uh i'm not 100 like i don't remember this erc from top of my head um i'm not sure if this byte4 is supposed to represent the selector of the function or a code that is like decided elsewhere or whatever but the syntax is same for solidity function selectors as well a bytes4 variable where um like which is basically a kcac hash of the signature of the contract which is this uh and we take the first eight digits or first four bytes of that hash that becomes the function selector in solidity so for higgs um guys i have hex in extras but i won't be able to get to that unfortunately i'm already i think going slower than i was anticipating i want to cover these smaller tokens right now um hex i briefly looked at it is 3000 lines of code if i start reviewing that it will take longer than i intend to run this stream for one bug please i think i just found a critical bug in andrei's smart contract that should be good enough how is un256 physically stored that is a weird question synchron so physically um it really depends on the software you are running um ethereum does not maintain mandate how it should be stored physically um so it really depends on the client that you are running from the avm perspective which is the ethereum's virtual machine um they have like 256 bit storage slots only so everything is stored in 256 bits and everything is like encoded as bytes and stored in that storage slot um you can like call it raw bytes bits or whatever but everything is 256 bits in evm this stream i will make it available in my videos once it is over um so the plugin for these arrows is i think solidity visualizer from tintin memory is more expensive than storage so that is not true um changing uh variable in memory costs i think 100 gas while uh storage right is 20 000 gas for a new variable storage rate is uh 2100 gas if i remember correctly per variable while in memory it is maybe 50 gas or something like that um as for gas optimizations i'll talk about the whatever points i see in the contracts um other than that i have a couple of blog posts that you can check out on mother.blog where i like gave some solitary gas optimization tips a couple of years ago but most of them are still valid how is divided by zero handled um that causes a panic in the evm it causes a revert all of the state is reverted well i see it okay yeah this is the same like someone mentioned that the reward distribution bug is the same as the one that was in popsicle and you are right this is the same bug or not exactly the same but the similar class of bug that was present in popsicle finance i did a twitter thread on that if you want to read about it i went into more details about it but yeah this is the same class of bug okay two times a week uh i will try to do more streams in future like this but uh i'll be honest it will be hard for me to find time to do uh two every week um i'm like this thing i'm just doing uh like out of passion or something uh so i don't want to commit to it uh really i will do it whenever i get get time but i don't want to take on any commitments at the moment i'm happy that you guys are liking it can we replace the four withdrawal and deposit into one with if else case yes you can the reason you split it into different functions is that way it is uh like 50 gas cheaper you are saving one uh like if else um loop or whatever so um yeah that is like the reason for splitting in four function is some gas optimizations only um the analysis for that actually let me just open up my twitter and i'll post a link here okay i think it that will uh i'll post my twitter link in the chat you can scroll down and find that analysis i don't want to like waste everyone's time on the stream looking at my twitter timeline uh but uh posted my twitter channel and your twitter handle you can scroll down and find that thread there okay i think i've caught up with the chat now let's move on to the next contract paywall dot sol um so now uh i'm i'll try to go on a bit faster um it's 10 30 pm here and i want to end the stream before midnight um so yeah i will be going a bit faster so some context a lightweight payment contract for access to digital assets you can create paywalls for articles courses under digital content where users pay with their ethereum polygon wallets in this case so yes this seems your like normal payable where to access some content you need to pay up um so most common example i think will be news websites like new york times um they offer you like some free articles per month and after you have consumed them you have to pay a membership fee to access more content the block has a payroll like this medium has a paper like this and a lot of folks have a paper like this it is definitely an interesting idea to create a decentralized paywall where people can pay via like their ethereum wallets or whatever directly so let's delve into this contract um so this variable i suppose this contract keeps a fee of the uh like pay wall so if the user if the content creator sets the price as one ether this contract will keep like 0.1 ether or something as free and or give these remaining 0.9 to the content creator again this is just an assumption i've created from this line from this comment and line i will verify if my con assumption is correct later on um since i don't have a lot of documentation go off to go off from i'm just creating a mental model in my mind itself to like just coming back to the my audit methodology that i described before i try to create a model of the contract before i actually look at the contract a contract fee a fee accrued by the contract contract has access so i guess this is check like this is set after someone has paid up so it gives them the access so we have multiple owners start score okay so this id basically represents your content creator id and this address is the content creator address this is depending um money that the asset owner has not withdrawn this is also mapping so on content creator id to the amount they have not withdrawn yet and this is the fee amount and this is the paywall amount that the user has to pay uh again this is from per contained creator id and to the amount that the user must pay that's all good just an event we'll ignore that the only asset modifier is fine um so another quick tip about solidity the whole reason string that you enter here is stored in the byte code of the smart contract so if you are like an ethereum evm also has a limit of 24 kilobytes on the size of bytecode of one smart contract so once this contract compiles down it can only be a maximum of 24 kilobytes or you won't be able to deploy it since this whole string is part of the bytecode this increases the size of your contract so if you are ever in a situation where your contract is exceeding the size limit of ethereum you can shorten this string to fit into one storage one slot which is by 32 or 256 bits just count 256 uh bits from here or like i think uh every character will take eight bits um you can count it from that um so anyway since a evm or solidity also just works on 256 bits at a time if you enter only o here you will still be using 256 bits in your storage in your space if you enter 256 bits you will still be using 256 bits if you enter 257 bits you will be using 512 bits of storage so first try to make sure that the error fits inside 256 bits um that's cool and if you are even like if you want to save even more gas just get rid of this um string completely and now you have saved even that like 256 bit okay um so create anyone can call create i guess this creates a paywall counter is auto incremented the amount of counter so this is your like contained creator id it is set to the fee that the user sets the owner of the counter is set to the owner and that's it so this function remain looks good grant access so i guess this is the function that the user calls to get like to pay the pay wall fees you provide the content creator id you provide the address um of the contract that should gain access so this means you can pay for someone else as well if you if the contract had used message sender rather than this field then you would have been paying for yourself only so this just checks that the counter exists the message value is ensured to be exactly the fee amount can the user change the payment okay so that's fine um this ensures that the address does not have a does not have permission already um so one like code quality improvement here rather than equal to equal to false maybe this is personal opinion i just do um not here um this means the same thing but like to my brain this just looks cleaner um under the hood i think solidarity will compile both of these to the same there can be one potential in like optimization here where they don't push false to the stack but they will be pushing close to the stack i'm sorry i'm just thinking out loud so i think the underlying code will be the same but that's just like personal opinion so here we calculate the contract fee amount um so we multiply the message value with the contract fee and we divided by the fee base this is the maximum fee or like 100 percent fee will be this value and this is the actual value we do this because we can't enter numbers in decimals in solidity so um if we wanted to take 0.1 percent fee um then you can't enter 0.1 here what you will have to enter is 1 here and 10 here that gives you 1 by ten which is actually ten percent not zero point one percent uh but you get the point you can't enter decimal points or percentage here okay so then we have the amount that get that is given to the owner we are doing a sub uh from the message value we reduce our fee and this is what the owner or content creator gets we increase the content creator spending amount by this and contract fee is accrued by this contract we accrued and we grant access to the address um to the user everything seems fine uh just one thing one issue potential issue i have seen here is that the if this contract fee is modifiable by the contract owner then they might like start their contract with zero percent fee and at a later date just modify it two hundred percent when the users are like buying in the content the user will not realize this because they will still get access and the content creator might take a few days or weeks to realize that the fee has been suddenly increased to 100 percent so this is another case um i will suggest if this variable is changeable it should be changeable via only a time logged multisec so here we can see the owner can change this variable this fee so um to this project my advice will be to make sure that uh okay so i suppose this inherits unable here my suggest my advice would be to make the owner a multi-sig and that to a time logged multisig so that um any change that you do is like it goes through a waiting process or something like that to avoid these like sort of rapport situations not exactly rugball but something similar situations okay let's continue back on here one we have modified one we have identified one issue that uh here that's fine uh withdraw allows the asset owner to withdraw funds we require that the funds should be withdrawable now we get the asset owner address we get the amount of withdrawal we set the pending withdrawal to zero and we transfer the amount so this is good um they are doing the changes to storage first before doing the transfer so even if this call was to re-enter uh now it will see that the pending withdrawal is already zero and cause a revert so um that's good no issues here you change the asset fee um this we have already talked about uh oh no this one is different so this is the fee that the user pays um this one i think is fine it can be 100 dependent on the content creator because it's a mutual agreement between the content creator and the user how much fee they are willing to pay there is no third party involved and therefore we do not need a multi seg like or a time lock basically obviously for the asset owner it's best to use a multiset for these functions but we don't a third party does not need this the situation where you need a time lock is when your changes affect a third party in this case they do not they only affect you and the buyer of your content so uh we don't need time locks here change asset owner same thing um don't need any uh we don't need any uh anything else here contract fee we have already talked about and contract withdraw only owner can call this is mine fake crude must be greater than zero and again they have done the right thing here by setting the variable to zero and they transfer the interview so this is an interesting hack um by the way if you do not have this receive function at all solidity will automatically cause a revert um you don't like need this function um so the author author's intention here is that people don't directly send any ether to this contract they always send ether by calling this grant access function i think they are on the right track but maybe a bit too verbose um so while we're talking about this some more smaller things you can do if you want to use require you can just put false a rather than zero zero equal equal to one and uh if you want to cause a revert you actually don't need require you can just use revert directly this is another function in solidity which just causes a revert you don't need to pass anything just like you don't need to check any condition pass your condition here and it will cause a revert and forward this condition so yep that is it i think paywall review is done nothing critical found just one like anti-rapport suggestion to use a time lock contract for the owner okay i'm gonna look at the comments now so on diamond uh storage like it does solve your issue of 24 kilobytes but it does inc like it adds extra complexity and gas cost to your call i don't have anything against using it but it is like not a one solution fits all situation in some in some scenarios diamond proxy is useful but not always best practices for fallback and receive um so not really the best practice honestly is to not have a fallback or receive function which means that if someone sends um ether to your contract mistakenly it will cause a revert but other than that like if you need a receive function or if you need a fallback function there aren't really any uh like best strategies or whatever for it it really depends on the use case that you want okay let's move on to the next one um so we have a time lock function this is what i've been talking about for a while now um so um access control i'm gonna assume this is just a standard contract from maybe opengl or something i'm gonna ignore that for this review we have revert message extractor this is another library we are going to ignore his contract library um so this make sure that the contract has been deployed in some private prior transaction so while i'm here i'll just add one comment about it um yeah this line basically so this is not a foolproof method you can bypass this check by making calls in the constructor of your contract deployment so if you are deploying a contract and in the constructor you call a call another contract and then that contract uses this library to check if you have if you are a contract or if you are an end user it will get an incorrect result because your contracts the one with the constructor from where you are calling where you are deploying on the blockchain this has not been deployed yet your con code is still zero although you are calling from a contract but it has it will be deployed only after the transaction ends uh it is currently being processed for deployment so in those scenarios this these checks can be bypassed so um like people should be like aware when using this check in their contract that is like an attacker can still use a contract to interact with your with your this thing time lock contract or whatever let's move on so we have grace period minimum delay so this is your execution delay i guess um the minimum number of days you must wait before executing a proposal maximum number of days you can wait before executing grace period um i guess it is like it must be 14 days extra on top of maximum delay but we'll see later delay is stored in argument 32 another quick tidbit about evm if you are not packing a uint together with anything uh you can see this is a un-32 so it can potentially be packed with other storage variables but it is not being packed so it is actually a bit gas efficient to use un-256 rather than q32 because evm always works in 256 bits so even if you provide just 32 bits to it it will have to do that conversion and that conversion will take just a little bit of gas or maybe like 10 15 units or something it takes a very small unit like amount of gas but it does consume extra gas um so it is best to just use joint 256 when you are not packing your variables similarly if you are using variable in a function in memory then again same story applies you should use you in 256 here like if you are defining a variable here um you went 256 something you should be using you in 256 rather than joint 8 or whatever even if you only need unt8 because in memory as well um solidity at least it used to i'm not sure if they changed it recently or not but solidity always assigned 256 bits so if you used a variable smaller than 256 bits you were just making um the avm do more work in those like conversions and all so it's uh better to use 256 bits directly not a security concern just a minor gas optimization thing okay we have a constructor that gives away a bunch of roles so these are the function selectors and the governor i guess gets most of the permissions and changing the delays can be done only by this contract not the governor this means that the governor can't just bypass the delay by changing the delay itself the governor will have to create a proposal in this contract itself wait for two days to execute for it to execute and that proposal will be able to change the delay in this contract so this address also gets the root access so this a proposal can call anything and the message sender i guess by default access control gives root access to message center we are revoking that that access here let me grab some water okay set delay auth is from the access role i'm going to assume that this auth modifier works correctly and has been implemented correctly in an audit again i would have verified this but for now let's just assume that is correct which means that only this address should be able to call the set delay function um we just do some checks and change the delay so no problems here then we have a proposed function that takes an array of addresses to call and data to call data to call these addresses with again we have auth permission so only the governor should be able to propose and it returns a tx we call an internal proposed function then we have a propose repeated um okay so this adds a salt um so that the tax hash that you get from the same data is now different this cause the same underlying proposed functions no problems here so one gas optimization you don't need the function modifier in all three of these auth you can have this only in this internal function and remove it from here or actually you can remove it from here and only have this here either is fine like either you remove it from these and just have it here just have it here or you remove it from here and just have it here i say that because this is an internal or private function that this cannot be directly called anyway so there is no reason for us to check permission at two different places uh here we first check that the user should have proposed permission and then we check that the user should have underscore proposed permission which i think is a bit like unnecessary um you can only check if that if the user has underscore proposed permission or you can check that the user has proposed permission so just one gas optimization there um so yeah this check verifies that the target's uh array list is array has the same length as the data length uh data array length one way to avoid doing this check and i think a neater approach overall is to create a struct which has a target and a data so we define a struct as something like this where we say we have an address and we have bytes data and then the input rather than being these two different areas the input becomes ram all data now you can just iterate over the length of this param and get the data and target for every eye the advantage you don't need to do this check solidity encoding decoding naturally mandates this check which means that this car is likely going to be a bit cheaper but only by a bit then um another check another advantage is i think this just looks cleaner which is like a personal opinion but yeah i prefer this approach over multiple areas uh moving on this calculates the k check or what however you pronounce it hash by encoding the targets data and salt that's all good now this can be modified by the proposer um i understand that like since this is in array you just change the order of that array and you will get a different hash although the actual elements will be the same which means like since we are already trusting the governor i think this is not a problem this is fine but if it was an open system maybe this could have been an issue then we ensure that the tx hash is already an unknown proposal that it has not been proposed already we set its state to proposed that's all good and we emit some events then the approval is called after the propose has been called so once the propose has been called we call approve to approve this proposal so we ensure that the state is equal to proposed this approve can only only be called by the governor again nobody else can call this and um state is um set to approved uh the eta when this should be executed is set to timestamp plus delay this is the minimum time after which the thing can be executed so above like i can correct my understanding here these are not the delay that are currently present in the smart contract these are the minimum and maximum values that the governor or this contract can set as delays the delay is a fixed value that is defined here and that means this grace period is delay plus grace period when you can execute the contract when you can execute the proposal okay so the ata is set state is approved um it is set here you can save a few units of gas by just saving eta here directly um even then this should get packed so that's fine proposals tx hash is set to your whole proposal okay so an alternative approach here that wait actually okay so that is fine um you're using memory here and that is why you have to write it separately i don't think i've seen the proposal struck oh it has a state and it has an eta so no additional packing can be done here both are packed together this is all good execute again takes the same data and calls the execute function this again can only be called by the governor and the underlying function same optimization can be done here we only need auth on either this underscore execute or both of these two functions okay we can we calculate the tx hash live we fetch the proposal details we ensure that the proposal is approved and we ensure that the block timestamp is more than dt and less than the grace period plus eta that's all good and that and then we delete the proposal um so that this can't be uh like once you delete the proposal you can't re-enter with the same proposal and we just empty up some storage space we create an array of all the results we i trade over the targets one thing i'm just thinking or something um so here um these guys are not using his contract to stop contracts from like doing some stupid but they are using his contract to actually as an additional check that we are creating proposals and calling contracts only so even like a contract bypassing this check here doesn't make sense and this is a perfectly valid use case of this check so for all the targets we do a low level call we call data we copy the data we cop we call the target if it's not a success we cause a reward if it is a success we put the result in the results array and at the end we emit the executed event so no problems here i'm just thinking about some attack vectors here that is like if you can somehow enter re-enter a an important function within the execution of the proposal um we would have been able to re-enter if this delete wasn't here but since this deletes the state even if you re-enter and uh like propose something it will add it as a new uh new proposal which is unapproved and like the 80 is this thing like when you approve it the eta set so from a quick review i think everything is working fine just one code that i would point out is i think since only the governor can both approve and propose it would be nice to have a single function called propose and approve or something like that where within a single function you can both propose a proposal and approve a proposal rather than having to make two different calls not other than that this contract looks good um it is high quality code so yep nothing else nothing obvious here anyway looking at the comments now okay so this time i have a bunch more comments most of them are about a certain project that i'm just going to ignore um i honestly have not looked into that project to make any like comments or whatever i'll just start this um like it's nice to be uh skeptical of new projects um you should do your own research before putting your money in do not trust what influencers say um influencers are really honest in my experience um so um it is nice to see that people are skeptical um but i'll keep i'll leave that at it is that i leave that there because i don't have a clue about what hicks is i i've seen them everywhere but i just don't want to look into it honestly i haven't got the time to do that moving on to the next contract we have this ubi burner um so this was the comment i got here this exchange is okay so it it swaps one token and freezes does it freeze the eat i guess so i guess what he's trying to say is they are trying to say here is in this function the user will provide a token the contract will swap that token for it and then the contract will keep that eat now let's see if my assumption is correct this is the interface for uni swap v2 router this is the router address with address ubi token address and this is the path now if it was a full audit i would have verified that these addresses are correct but again for now i will skip that so burn ubi we have this function here get amount of this is a view function this is a view function and we have a receive which just receives ether um so yeah the user was right this is the main function we want to look at and yet this is a like a small contract so it should be it shouldn't take long to review they also have friendly comments so i'll read that by you payments okay so they are buying ubi token they are not selling ubi token interesting interesting so um they are not selling the token they are buying it open so people are supposed to somehow somehow this contract is supposed to have ether in it and people are uh anyone is allowed to use that ether to buy this ubi token and that ubi token will forever be logged in this contract i'm not sure how this contract gets ether it can't get ether in this call because it's not a payable function so it must be getting ether from an external system and we're just going to um ignore that fact and assume that there is an external system that funds this contract so with that assumption in mind that this contract is already funded we are not taking any funds from the user let's move on to the next assumption next thing so this is that minimum okay this is not good but so let's first look at this function what it does is the interface the first parameter is amount out minimum this is the minimum output token that you are willing to get so if one if the pool is um let's say at a state where one ether costs 3000 usd and if you give one ether to the pool you would want to get 3000 usd back so you will put amount out minimum minimum to maybe like 2999 or something like that just give some leeway to the contract in case the price changes before now and try transaction gets mined but you make sure that you at least get most of what you were expecting then this path is like whatever you are swapping with to e width to ubi path implies you are putting in width and swap and getting up back two is the address that gets the output token and this should be addressed this in our case and deadline is the last date you can execute um this one it should be anything equal or more than the current block address um so this user is taking the deadline as as a user input which is fine i guess but they can also just hard code it to something high value like they can use block timestamp plus one or just max q and 256 because since the user is not controlling the price the deadline is not useful for the user the amount that's fine the amount of each that should be put in um okay so this just caused this swap function and here i'll explain what the issue is now if you set the amount out min to a very low value then you open yourself up to uh to the sandwich attacks basically where someone unbalances the poo so if you are in let's say each usd pool every eat and you want to sell it for usd eats right now is worth three thousand dollars so um you get three thousand dollars for selling one eat but if someone unbalances the pool by selling a bunch of each um in the router um they will sell a lot of heat they will get that eat via a flash loan maybe or they have some big pockets they will unbalance the pool and now a pool will be in a state where one eat is worth let's say only one us dollar um it has already sold all its seed they will now include your transaction in the block they will execute your transaction which is this burn upi now instead of getting three thousand dollars worth by selling your vanit you are getting only one token back so this like you would have expected to get 3000 but you got only one token back you have basically lost money here you can't do anything about it and ending in the in the second step um they will buy back the eat for the cheap price now since you have also sold each the price of each has now actually gone down further uh if they left it at one dollar you have made the eighth price to go to maybe 0.99 so now when the attacker starts buying each they will start buying at a lower price and they will be able to make a profit out of your misery um since they are buying it for cheaper than they sold it for at the end they make a profit and that profit basically comes from the loss that you have made by selling your tokens for cheap the caveats here is that unbalancing a unipool like this or sushi pool like this requires you to have a lot of liquidity a lot of like funds um you can take a flash loan or to like for those funds and in fact right now keeper dao is offering free loans you can borrow like 10 000 um ethereum i think they have 10 000 million dollars sorry i missed the numbers i think they have 10 million dollars worth of ethereum in their contract right now that they are giving as flash loan for free obviously you need to return that each within the same transaction but you can use 30 to do these arbitrage transactions um so that doesn't cost you anything but one thing that will cost you is the fee in the uni swap pool itself uh in v2 the fee is i think 0.3 percent so when you take the price down uh you pay 0.3 percent in fee and when you bring the price up you again pay 0.3 percent fee so you are in total paying let's say about 0.6 percent in fee so the profit you make should be more than 0.6 percent so um the amount that this function is swapping should be more than that fee for the attack to be like feasible for you to execute or else you will also lose money this user will also lose money only the uni swap lp providers will gain money in terms of fees um so if those caveats are fulfilled um this is ripe to being exploited in like in sandwich attacks since this function can be called by anyone people don't even need to do it in different transactions they can create a single transaction which takes a flash loan unbalances the pool calls this burn qpi and finally rebalances the pool and then they also pay back the loan and keep the profit with them so yeah i think this needs a fix um so how can you fix this uh that's an interesting question if you want anyone to use your yeast to burn up you one way to fix it will be to use an external oracle like chain link to make sure that the price you are getting for ubi is valid or you can use the uni v2s uh that is a bit more complex but you can use universe uh devops i as well and make sure that the price has not changed significantly in the last a few uh like in the last block or something um that will also fix this issue another option will be to make this function like permission and uh take this as a user input so you as a user provide the main value so that you cannot get like um sandwiched easily um you can't if you can't make this a user input without making this a permission function because then the attacker will just enter one from their side um so yeah this is uh although the contract is simple this has a one of the like d5 cable function or something like that this is like one of the common d5 pitfalls a lot of uh projects have fallen uh like ill to these kinds of flash alone oracle manipulation and swap attacks in the past especially on the finance chain um that being said if you expect to only be swapping like 10 worth of tokens on every transaction then this is totally fine nobody will spend the gas or the funds required to do a uh sandwich attack for maximum ten dollars of profit so if a lot of things depends on your risk appetite and how you use this contract but it is recommended to fix this i'll now look at the comments again you see still people are talking about ponzi i'm gonna ignore those comments is keep at all more gas efficient than dydx i have honestly not compared their gas efficiency but one thing i will say this is keeper dao is easier to use um especially when it comes to documentation or something um i briefly looked at dydx like last week in fact when i was rescuing uh like i was in a war room for misu and dydx documentation was a bit more like involved or like complex then keeper and since i wanted to get things done i just went with keeper by the way this is a good one dydx also offers free loans i think you have to pay back one extra way in that case but technically it is free in keeper dow you don't need to pay back any extra weight you can pay back exactly the same amount at least right now keep it up i think has a function to increase the fees in the future but right now the fees are zero okay let's move on to the last contract last but not least this is yield token compounding so i actually know about this like project already so if anyone knows about element finance this is built on top of that element finance has some yield tokens and some principal tokens yt pt that you get by depositing your assets so if you deposit your asset you get a portion of pt and a portion of yt so what this project does is it deposits your assets into element it gets some yt and some pt and then it sells those pt for the underlying token then it again deposits those underlying tokens into element and now it again gets some pt and some yt it keeps the yta uh away it keeps the yt safe and it reinvest excels the pt for underlying token and keeps repeating the process so it is like compounding your yt holdings it is basically converting all your um like all your underlying tokens to yt rather than a combination of yt plus pt so immutable addresses that's all good this is the main compound function we'll come back to this later an internal function internal function another public function okay anyone can make this contract to approve the balancer wall to spend any token basically so this balancer vault should be like this balancer vault should be an immutable contract uh because this balancer vault can steal all tokens in this uh contract so this balance evolved once this contract is deployed people should verify that it is set to the right address and like it can't steal funds from this contract so that's my number one point and just to repeat using this function you can give the balancer vault permission to spend all your assets um so the balance of vault should be a legit contract which does not spend your funds i don't have the balance of all source code here or else i would have verified if balance of world can like is there any back door there or something which can be exploited this contract actually shouldn't be holding any funds anyway so this is fine i guess this just compounds the tokens and returns you everything okay so the compounding count should be between 0 to 31 not 1 2 to 55 so your code comment or your code is incorrect one of these two things most likely the comment is incorrect because you will if you do it more than 31 times it will likely just go out of cash okay they have decided to skip permit for now that's fine okay the smart contract uses transfer this is just getting the underlying token from trans base token address um so this was this is the underlying token i was talking about and this gets the initial balance of the user position address underlying transfer from so this transfers the underlying token from user to this directly sends it to the position address that's good um this is this basically starts the deposit function will later be used to start the deposit function in element we are sending the underlying token from user to element um and then eventually like later on we'll get we'll get yield token and pt from uh element and we'll again sell that pt for underlying repeat this process to mint more yt and lt and stuff like that so yeah we are doing that in a loop here i'll enter the loop later but this loop returns a yt balance amount so the yield token the user would have gotten and this ensures that the final result is more than the expected amount user expected this avoids slippage and the sandwich attacks that i just described in the ubi burner that's good and then it transfers that uh yt balance back to the user so yeah this contract does not hold anyone so all good and we also verify that the output is more than what the user expects okay so this i'll say is like it's fine if you are returning this data but this is just like increasing the gas cost for no real reason um so yeah just one thing to point out now let's enter the loop now one thing i'll say is the indentation this is not the standard indentation um the solidity guidelines are present in the dots um they will tell you to indent your code something like this this does look cleaner if you ask me no bug here or whatever is just like your like guidelines uh for writing code you can use the prettier uh plugin or it has a solidity add-on and that will automatically format your solidity code i think it's quite nice and uh will save you from like manually uh doing all of this okay back to the code we start with white to balance equal to zero we start a loop here the same point you went eight you can use un-256 instead and save some gas joint 256 256 pre-funded deposit so since we have already transferred the tokens we are calling the pre-depo pre-funded deposit and we are the yield and pt token should be sent to this address yt balance is increased by the yt we get then we swap the pt or the underlying asset i suppose so if it is the last loop um if i equally equal to n minus 1 that's cool the next loop will be i equal to n and that will break so yeah this is the last loop send the remainder to message sender else send the remainder to this trance address or like wrap position address which will help you like compound on the effect it's interesting that the wrap position address is different from tranche anyway the waitress balancer pool id this is hard coded i suppose oh no it's provided as user input base token address this is the one you want it is and pt this is the token you are selling so um everything cool in this you are selling your pt um and you are getting uh you are selling your pt getting the base token and the base token is directly going to the tranche where in the second loop you will again create pt and yt out of that base token so this loops this loop looks fine this is um overall like you could do sandwich on balancer but we don't have an issue here because we have this check even if someone tries to like do a sandwich on the balancer this check will eventually fail when the out resultant output is less than the expect expected amount and uh will be golden like the sandwich attack will be stopped so i think that is all good return the yt balance finally i am just verifying that we are only using message sender here and like no other address other than message center should be able to call these functions yep only message center can call this so that is also good swap it is for base token on balances so we call this the exact thing i was looking up i think i found here the this is a public function and the from address is a user input rather than being rather than being uh rather than being message sender so this is a problem because um as you have seen the user will be approving the tokens here um if they want to use the compound function naturally okay so i have one concern about this contract do we have the authors of this contract online i want to confirm if this is live or not i don't want to talk any i want to i'd like i don't want to talk more if this is a live contract i'm looking at the chat if the people are online this is not an element phi contract it is created by a third party but since i'm not sure if this is a live contract or not i will um leave this uh as it is but yeah i think there is a potential bug is in this contract so um anyway we have covered this contract this has bugs this one has bugs this one does not have bugs but has some minor improvements they've all i can't remember oh yeah this has the concern around only one no bugs just one concern options lms has a critical bug bridge deposit um is a basic contract where you just trust the owner so no bugs but you must be trusting the owner um so as you can see like if you pick any random contract from the internet you are very likely to find some bugs and some critical bugs in those um and that was it i think i've uh this stream has gone for 2.5 hours i would uh look at the chat for a bit if anyone has any questions or anything i'll answer those stick around for maybe five more minutes and then we can call it a day meanwhile i should just message these guys on twitter if this contract is live if they come back to me within like a good time frame looking at their github repo doesn't seem like the contract is like they only have girly address in there but i am still like i would wait for like their reply before i talk more about that contract okay um i think we can uh call it a day for there um this contract i will likely like once i get a response from the authors um i will probably just post a twitter comment or something like that describing debug or this actually you guys can take on as a challenge or something i think i've given enough hints in the call itself to be able to figure out what's wrong with this contract and like in future you guys can avoid that again i'm not 100 verified uh debug i just have a feeling that there is a bug um i don't want to verify it while being live okay thanks everyone thank you for coming to my live stream honestly i think it went better than i was expecting for my life for stream i was totally expecting there to be at least a dozen technical issues or whatever but maybe youtube is more mature than i was uh expecting it to be um this is gonna be the outro have a nice day everyone and we'll probably uh talk soon cheers bye