The developers of the YungApes Token contracted byterocket to conduct a smart contract audit of their token contract. The token is “going to work as a reward for YungApes NFT holders, and will provide additional functionality to its owners.”
The team of byterocket reviewed and audited the above smart contracts in the course of this audit. We started on the 13th of April and finished on the 2rd of May 2022.
The audit included the following services:
byterocket gained access to the code via a public GitHub repository. We based the audit on the main branch’s state on May 1st, 2022 (commit hash a3a0b475c814c5d16f87abafad8e00b7be19a124).
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 9 bugs or flaws, with 9 of them being fixed in a subsequent update. Prior to this, there have been 7 non-critical findings, 1 of medium severity and 1 of high severity.
The contracts are written according to the latest standard used within the Ethereum community and the Solidity community’s best practices. The naming of variables is very logical and understandable, which results in the contract being easy to understand. The code is well documented. The developers provided us with a test suite as well as deployment scripts.
Location: Yapes.sol - Line 36
Description:
The changeBenefactor() function is replacing the address of the tax recipient with a new one. However, it seems like the onlyOwner keyword has been forgotten. In the current implementation, anyone can change the benefactor address at any time.
These are the affected lines of code:
Recommendation:
Consider adding the onlyOwner keyword or another way of controlling the access to the function.
Update on the 20th of April 2022:
The function has been changed to include the onlyOwner keyword.
Location: Yapes.sol - Line 231
Description:
The transfer tax is defined as 15% of the currently transferred value whenever a person is not on the noTaxWhitelist. The corresponding tax itself is calculated in line 218, where it is stored in the tax variable. When removing the total amount from the sender, the calculation is “total_amount = sending_amount + tax”. However, when transferring the tax to the benefactor address, only a third of the tax is sent as tax/3 in line 231.The result is, that two-thirds of the tax value is being removed from the sender balance but not accredited to anyone, which also renders the totalSupply value incorrect (as totalSupply should be the sum of the balances of all of the addresses).
Recommendation:
Consider correcting the calculation to not divide the tax value by 3, if this behaviour is not intended.
Update on the 3rd of May:
The developers explained their fee mechanism to us in subsequent discussions. Their idea behind the fee was to only send one-third to the feeWallet and burn the other two-thirds with the intention of sending one-third to the community later on via a manual minting call. The developers have implemented a fix so the totalSupply is properly adjusted.
Location: Yapes.sol
Description:
A standard best practice is to emit an event whenever a global variable is changed. This allows for a much simpler indexing and historical analysis of important value changes over time. Currently, there is no event emitted for changes to the whitelist (add and remove), for changes to the minters (add and remove) as well as changing the benefactor.
Recommendation:
Consider adding events for any state-changing functions.
Update on the 3rd of May:
The developers have implemented events for every global state-changing variable.
Location: Yapes.sol - Line 218
Description:
For global values that have a direct impact on the economy it is usually a good practice to include an option to change them later on as the project matures. Currently, the tax values are hardcoded at 15% and can never be changed.
These are the affected lines of code:
Recommendation:
Consider adding a function to change the fee as the project matures.
Note: If you decide to add a function, we would also like to point out that working with basis points instead of percent will give you a greater precision (e.g. the ability to use half-percent values etc.).
Update on the 3rd of May:
The developers have acknowledged this finding and are not intending to change the fee later on. Leaving it hardcoded, in that case, is perfectly fine.
Location: Yapes.sol
Description:
Throughout the contract you are switching between using msg.sender as well as _msgSender(). There is no clear benefit in using msg.sender if you import the Context library (via Ownable) unless you would want to limit future use of meta transactions.
Recommendation:
Consider using _msgSender() in every case instead of using msg.sender in some instances.
Update on the 3rd of May:
The developers have switched to using msg.sender in all places.
Location: Yapes.sol - Line 269
Description:
Within internal functions, it is usually best practice not to use a public function if there is an internal version that is cheaper to use. In this case, using the `allowance` function is more expensive than using the _allowances mapping.
These are the affected lines of code:
Recommendation:
Consider replacing the allowance() function call with _allowances[owner][spender] to save gas and to adhere to the best practices.
Update on the 3rd of May:
The developers have changed the corresponding line to our recommendation.
Location: Yapes.sol
Description:
As calling external functions is cheaper than calling public functions, it is best practice to mark any functions that are not used within the contract as external.
These are the affected functions:
Recommendation:
Consider changing any public functions that are not required to stay public to external.
Update on the 3rd of May:
The developers have changed the corresponding functions to be external.
Location: Yapes.sol
Description:
There are five functions that are marked as virtual, which would allow them to be overwritten in a contract that imports the current one.
These are the affected functions:
Recommendation:
Consider removing the virtual keyword, if it is not required.
Update on the 3rd of May:
The developers have removed the virtual keyword in the recommended instances.
Location: Yapes.sol - Line 210 - 236
Description:
Tokens that take a fee from the amount of a transfer are often used, however, usually the fee is deducted from the amount. In this case, the fee is added to the amount. If someone has 50 tokens and tries to send these 50 tokens to someone, they will fail, since the _transfer function will try to send 57.5 (= 50 + 10% of 50) tokens. The documentation doesn’t reflect this.
Recommendation:
Consider adding a bit more documentation to the transferring functions to give the users a chance to find out why certain transactions might have failed. There might be users who are confused about this since it’s not how OnTransferFee tokens usually work.
Update on the 3rd of May:
The developers have added documentation to the transfer function and will be displaying documentation on their front end.
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.
According to our analysis, the protocol and logic are working as intended, given that any findings with a severity level are 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 9 bugs or flaws, with 9 of them being fixed in a subsequent update. Prior to this, there have been 7 non-critical findings, 1 of medium severity and 1 of high severity. 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.
