The team of dao.care contracted us to conduct a software audit of their developed smart contracts written in Solidity. dao.care is a project creating a platform to democratically support community projects with joint interest earned through staking a stable coin called DAI on the Ethereum blockchain.
The following services to be provided were defined:
We gained access to the code via the public GitHub repository via https://github.com/DAOcare/app/tree/master/contracts. The state of the code that has been reviewed was last changed on the 14th of April 2020 at 09:38 AM CEST (commit hash 99b3f553158d4fe3b85facf6461d313695efa188).
We conducted a manual code review, where we focussed on the two main smart contracts as instructed by the dao.care team: ”NoLossDao_v0.sol” and “PoolDeposits.sol”. For a description of the functionalities of these contracts refer to section 3.
The code of these contracts has been written according to the latest standards used within the Ethereum community and best practice of the Solidity community. The naming of variables is logical and comprehensible, which results in the contract being easy to understand. As the dao.care project is a decentralized and open-source project, these are important factors.
The comments in the code help to understand the idea behind the functions and are generally well done, but in some areas there is still room for improvement.
On the code level, we did not find any alarming bugs or flaws. The ones we describe below (2.1 and 2.1) are of low severity.
The comment is wrong, it has been copied from the function above (line 205).
Some form of frontrunning is definitely possible here, but is expected and desired. Just noting for completeness, more details in the protocol/logic analysis.
Technically these lines do the right thing, but the division allows for a slight remainder in the case of an odd number which is not considered. See the following example:
Let amountToRedeem be 1001 and the percentages be 13,5% and 1,5% as proposed by you. The divisions compute as follows:
First iteration of the loop (line 261): amountToSend = 135,135, but since uint256 is an integer, the place after the decimal point will be dropped without any rounding leading to a value of 135.
Same for the second iteration where we receive a value of 15 instead of 15,015.
Line 268 will now compute the amountToSendToWinner as 850 instead of 850,85.
In the process, the amount of “1” will be left and won’t be distributed..
The severity of this is very low. Since the DAI token is denominated with 18 decimal places as a 1018 value in uint256, this would mean that a value of 10-18 DAI is being kept without being distributed. The remainder would be distributed as soon as the amountToRedeem is even at the end of an iteration. There is no risk associated with this. If this is desired to be fixed, a possible solution would be to store and add the two amounts that are being sent within the for-loop (lines 261 to 266) and subtract this value from the original value of amountToRedeem. The result would be the rest that is being paid to the winning project, without leaving a remainder.
As the difference that this fix makes doesn’t have any real economic impact and probably introduces a slightly higher gas cost of the execution, there is no real benefit of fixing it.
There are basically two main groups of participants when it comes to the user-side of the protocol. Regular users and project owners.
Regular users can stake DAI tokens to receive voting rights in the same height as their stake and influence the vote, where one project is regularly chosen to receive the accrued interest of the users. The DAI that is being staked is automatically lended via the Aave protocol in order to gain interest.
Project owners can stake DAI tokens to propose a project to be voted upon.
The whole protocol works in so called iterations, where one iteration will take two weeks (not fixed in code, can be changed). At the end of each iteration the winner of the week before will receive the accrued interest. A project that has won will be put on cooldown, which will be removed after the next iteration. A project on cooldown can’t win another prize during that time.
On top, there is an emergency functionality, where everyone who staked can vote for an emergency. As soon as >50% of the users voted on this, the state of emergency can be activated, which allows everyone to withdraw their stakes.
Public Mining Function
While the functionality description covers most of the logic, there are two things that need to be explained separately: the voting mechanism and the fund distribution during the iteration change.
If a user votes on a project, their token amount is taken as votes and added to the counter of the corresponding project for the current iteration. In the event of a tie, the project that reaches the number of votes first wins.
Fund Distribution and Iteration Change
At the end of each iteration a function will start the next iteration and pay out funds to the last week's winning project. A winning project is called the ”top project”.
While we did not find any bugs during the manual code review, we do have some potential flaws/vulnerabilities that are potentially exploitable and might pose some risks on the protocol level.
When a regular user wants to vote on a proposal, it is ensured that this account does not have a project/proposal themselves (line 330). We think that this is a good measure to separate the two types of users and clearly distinguish between the ones who are giving and the ones who receive something. This check is also in place when someone wants to transfer their voting rights to another address, where both are not allowed to be a project owner (lines 314-315). However, you are not checking whether the person who receives the voting rights still doesn’t have a proposal once they are voting. In theory, the following can happen:
This can potentially lead to the abuse of the vote via proxy system, where users like Bob may promise to vote on behalf of other users in a good way, only to later create a proposal himself and then vote on it with their votes.
The easiest way to fix this would be to include another check in the voteProxy function (line 340). Therefore we propose the following change. Currently, the list of modifiers for the “voteProxy” function looks like this:
The following addition would mitigate the risk (changes in bold):
The function “distributeFunds” in line 385 to 431 is prone to frontrunning. Since anyone can execute it after an iteration has finished, the first one who submits their transaction will succeed. If the project gains enough traction, there might be people who apply scripts to do this automatically and outbid other people trying to do so by upping the gas price, leading to a mining-like process. We are aware that this is probably a desired property, since it definitely keeps the protocol running in a timely manner and incentivizes people to do so on a continuous basis. It is probably not the fairest mining mechanism, but should be sufficient to produce the desired outcome without posing any risks to the users. We are not flagging this as a vulnerability or flaw, just noting it for completeness.
The function “distributeFunds” in line 385 to 431 includes a segment where the last week's project receives their accrued funds. In lines 416 to 419 the project is being set on cooldown if it hasn’t been withdrawn already.
Potentially, a project could withdraw their project immediately before the iteration ends and they are sure to be winning. They would still win and could then go ahead to submit another project for the next iteration and potentially win again, not being blocked by the cooldown.
If this behaviour is not desired, its abuse could be blocked in two possible ways. First, you could introduce a check whether someone who aims to withdraw their proposal is currently leading the vote and thus block them from withdrawing. Another possible solution would be to check whether someone creating a new proposal is actually the benefactor of last week's proposal.
In your protocol, the winner of an iteration will receive the accrued interest of the following week. Vice versa, at the end of an iteration the accrued interest will be sent to the winner of the week before (e.g. winner of iteration 6 receives the interest of iteration 7 and so on). This could potentially lead to the following case:
Although we can see that this might be intentional, we also see a DAO as a democratic and solidary construct. The vote loses some of its credibility if the participants don’t need to act according to what they agreed on. If I enter the social construct of a DAO voting scheme, I am probably okay with the democratic nature of the fund distribution. Of course, it is okay to allow people to leave the pool at any time, but they shouldn't be able to influence the result afterwards through leaving it. In the current implementation they are essentially voting on next week's money, where they might not be part of the pool anymore if they decide to leave.
If this behaviour is not desired, it would be possible to give the current iterations accrued interest to the winner of the same iteration, equivalent to how projects like PoolTogether are handling it as far as we are aware.
Since your voting scheme is public and does not provide voting confidentiality, it is possible to see the current standing at all times. This could potentially allow people with large holdings inside the pool to wait till the iteration is nearing completion and then decide on the project that they are in favor of, especially if two projects are nearly tied.
A possible solution to this would be the implementation of a commit-reveal-based voting scheme, where the votes will be public after the iteration finishes but are confidential before this happens. We do see that this might now be desired and you might be okay with the public property and the risks that it poses. We also see possible implications for usability that might lead you to deciding not to use a commit-reveal-based voting scheme, since it requires the user to facilitate an additional transaction during the reveal-phase, which is not nearly as nice from a UX perspective. We merely want to draw attention to the implications of a public voting scheme and list it for completeness.
In lines 65 to 68 there is a fixed limit of 200.000 DAI that acts as a limit for the emergency state functionality.
Below this threshold, it is not possible to activate the emergency state. While there might be a rational decision behind this implementation, we would at least like to see an explaining comment for anyone interested in this. We personally do see a problem when just 10.000 DAI are locked in the smart contracts and an emergency occurs. In this case, none of the participants would be able to recover their funds. A very simple solution would be lowering this limit, which does indeed introduce more risks of abuse of the emergency function. A better solution would probably be a bit more complex, but we still suggest to spend some thoughts on an alternative solution, even if it is “just” for the next version of the smart contracts.
Overall the smart contracts are very well written and thought out. We like the idea of not recycling any foreign code in your core logic and writing it all by yourselves, since it enables you to understand every little detail and doesn’t introduce any unnecessary risks.
During the manual code review we did not find any alarming bugs or flaws. Merely the comments could be improved.
Our protocol and logic analysis did show five possible flaws. While none of them are posing a threat to the users funds, we see especially 3.3.A (“No project check for delegate voter”) as something that needs to be fixed before launching the dao.care product to mainnet. Still, the rest of these findings need to be taken into account and decided upon by the dao.care team.
Since we sent our report to the dao.care team, the findings have been discussed in a bi-lateral meeting.The team of dao.care addressed our findings and the implementation of these mitigations in this pull request on GitHub. Overall, the following of our found flaws have been addressed:
The comment has been corrected.
The error allowing for a slight remainder in the division has been fixed.
The necessary check (in the form of a "require") has been added.
As stated in the report and pointed out by the team, they are aware of this, as it is part of their design and is intended to be this way.
The dao.care team is not worried about this, as they don't think, that making use of this will be socially possible. We agree with this.
The dao.care team understands our concerns. However, they intended this feature the way it is implemented, in order to allow for "rage quits", which is understandable. We agree with this.
The dao.care team stated that adding a concealment mechanism for the votes would add a level of complexity to the system that they are not looking to introduce during this iteration. We agree with this.
The dao.care team states, that they like the large limit, but they introduced a new function that allows an additional set of admin keys to immediately declare the state of emergency. As this ensures, that in the case of an "early failure" of any of the systems everyone will get their money back as soon as the team declares the state of emergency, we think that this is a good way to handle it.
Two major additional features or modifications have been implemented since we audited the code in the context of this pull request: 1) Upgrade to Solidity version 0.6.10 and 2) the ability to partially deposit and withdraw funds. The main reason for (1) is to account for failures in the aDai contract, which is now being handled in a try catch manner. We checked the pull request that contained these changes and were not able to find any issues that have been introduced this way. We further used an automated code checking software (MythX) to verify that we haven't missed anything.
We store our public audit reports on IPFS; a peer-to-peer network called the "Inter Planetary File System". This allows us to store our reports in a distributed network instead of just a single server, so even if our website is down, every report is still available.
The IPFS Hash, a unique identifier of the report, is signed on-chain by both the client and us to prove that both sides have approved this audit report. This signing mechanism allows users to verify that neither side has faked or tampered with the audit.