The developers of Weave Financial contracted byterocket to conduct a smart contract audit of their staking smart contracts. Weave is “a DeFi protocol that brings expert yield farmers together with less experienced investors looking to make passive returns from cryptocurrency. The Weave dApp has a huge range of liquidity pools and staking vaults, offering auto-compounding to generate some of the highest yield farming returns you’ll find anywhere.”
The team of byterocket reviewed and audited the above smart contracts in the course of this audit. We started on the 14th and finished on the 18th of February 2022.
The audit included the following services:
byterocket gained access to the code via a private GitHub repository. We based the audit on the main branch of the repository, deployed on January 30th, 2022 (commit hash f79bfcf34cfe0c742be8d48a98c6aea289256d7a). The updated version was provided to us on March 4th, 2022 via two new commits to the private repository, addressing our findings. The last and most recent commit hash that we audited is 39e49715579a47921e8940f5ca0aaff05dfd9cef.
We conducted a manual multi-pass code review of the smart contracts mentioned in section (1). Three different people went through the smart contract independently and compared their results in multiple concluding discussions.
The manual review and analysis were additionally supported by multiple automated reviewing tools, like Slither, GasGauge, Manticore, and different fuzzing tools.
We are categorizing our findings into four different levels of severity:
On the code level, we found 8 bugs or flaws, with 8 of them being fixed in a subsequent update. Prior to this, there have been 6 non-critical and 2 low severity findings.
The contracts are written according to the latest standard used within the Ethereum community and the Solidity community’s best practices, with some exceptions listed below. The naming of variables is very logical and understandable, which results in the contract being useful to understand. The code is well documented. The developers provided us with a test suite as well as deployment scripts.
Location: Staking.sol - Line 267 - 314
Description:
There is no check to ensure that the amount parameter of the deposit() function is not zero. Hence, it is possible to deposit nothing, which unnecessarily emits events and creates a deposit entry, inflating the user’s contract storage.
These are the affected lines of code:
Recommendation:
Consider verifying whether the specified amount is greater than zero.
Update on the 9th of March 2022:
The deposit function now includes the suggested require statement.
Location: Staking.sol - Line 276 - 280
Description:
Since there is a function setToken(), which replaces the used token in the call below, it would be technically possible, that an untrusted token could be used. If this token is implemented maliciously, it could lead to a reentrancy attack. The contract uses the reentrancy guard, but not in the deposit() function.
Recommendation:
There will probably be only one token that is being used here, which is a trusted and safe one. However, ensuring that no reentrancy attacks can be facilitated, it might make sense to use the reentrancy guard in this function as well.
Update on the 9th of March 2022:
The deposit function now uses the nonReentrant modifier to prevent this.
The variable feeAddress is not initialized, which leads to a race condition in which the function dealRedemption() should not be called before the feeAddress variable is initialized with the updateFeeAddress() function, otherwise fees taken by the contract would be sent to the zero-address.
Recommendation:
Initialize feeAddress in initializeInvictusFund().
Update on the 25th of January 2022:
The feeAddress is now being initialized in initializeInvictusFund().
Description:
Throughout the code, there are multiple for-loops that can be further optimized to save gas. As the loop goes through more iterations, the impact of this grows.
This is one of the affected loops:
Recommendation:
Consider changing your for-loops to be more gas-efficient. Keep these rules of thumb in mind:
An optimized version of the above loop would look like this:
Update on the 9th of March 2022:
All of the loops in the contracts have been optimized.
Location: StakingLP.sol - Line 435
Description:
In the Staking.sol contract, there is a require statement that does not occur in the StakingLP.sol version. This might have been taken out on accident.
These are the affected lines of code:
Recommendation:
If this change has not been done on purpose, consider adding this require statement back in.
Update on the 9th of March 2022:
The require statement has been added to the StakingLP.sol contract.
Location: Staking.sol - Line 557 - 574
Description:
The withdrawDeposits() function implies that it is used to withdraw the users deposits. In practice, however, it can be used in the special case of a user trying to withdraw all of their deposits fully instead of the most recent one, which will just be withdrawn partially.
Recommendation:
Consider finding a more appropriate name for this function.
Update on the 9th of March 2022:
The developers are aware of this and have been working on a more suitable name for this function. Since this has no security implications at all, it is now marked as fixed.
Location: Staking.sol - Line 272
Description:
Storing block.timestamp in a variable and reading from there is more expensive (3 gas units) in comparison to using block.timestamp directly (2 gas units).
This is the affected line of code:
Recommendation:
Consider using block.timestamp directly whenever you need it. Storing it in a variable is not saving any gas.
Update on the 9th of March 2022:
The use of block.timestamp has been updated according to our recommendation.
Location: Staking.sol - Line 275 - 281
Description:
In this case, it is impossible for msg.sender to be address(this), hence, the statement is always true. If a contract call leads to the call of another function in the same contract, this does not change the context, hence, msg.sender stays the same.
For this statement to be true it would require a special implementation of a function call (e.g. an external function that is called via the ABI of the contract). This is not the case here.
This is the affected line of code:
Recommendation:
Consider removing the unnecessary if-statement, as it will always be true.
Update on the 9th of March 2022:
The if-statement has been removed.
Part of our audits are also analyses of the protocol and its logic. The byterocket team went through the implementation and documentation of the implemented protocol.
The repository itself contained tests and documentation. We found the provided unit tests that are coming with the repository execute without any issues and cover the most important parts of the protocol. There are some unit tests, where the output is only being printed and not verified through an expect or assert statement.
According to our analysis, the protocol and logic are working as intended, given that the findings listed in section (2) have been fixed.
We were not able to discover any additional problems in the protocol implemented in the smart contract.
During our code review (which was done manually and automated), we found 8 bugs or flaws, with 8 of them being fixed in a subsequent update. Prior to this, there have been 6 non-critical and 2 low severity findings. Our automated systems and review tools did not find any additional ones.
The protocol review and analysis did neither uncover any game-theoretical nature problems nor any other functions prone to abuse.
In general, there are some improvements that can be made, but we are very happy with the overall quality of the code and its documentation. The developers have been very responsive and were able to answer any questions that we had.
As of the date of publication, the information provided in this report reflects the presently held understanding of the auditor’s knowledge of security patterns as they relate to the client’s contract(s), assuming that blockchain technologies, in particular, will continue to undergo frequent and ongoing development and therefore introduce unknown technical risks and flaws. The scope of the audit presented here is limited to the issues identified in the preliminary section and discussed in more detail in subsequent sections. The audit report does not address or provide opinions on any security aspects of the Solidity compiler, the tools used in the development of the contracts or the blockchain technologies themselves, or any issues not specifically addressed in this audit report.
The audit report makes no statements or warranties about the utility of the code, safety of the code, suitability of the business model, investment advice, endorsement of the platform or its products, the legal framework for the business model, or any other statements about the suitability of the contracts for a particular purpose, or their bug-free status.
To the full extent permissible by applicable law, the auditors disclaim all warranties, express or implied. The information in this report is provided “as is” without warranty, representation, or guarantee of any kind, including the accuracy of the information provided. The auditors hereby disclaim, and each client or user of this audit report hereby waives, releases and holds all auditors harmless from, any and all liability, damage, expense, or harm (actual, threatened, or claimed) from such use.
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.
