The developers of Senken contracted byterocket to conduct a smart contract audit of their settlement smart contract. Senken “strives to reroute climate finance to the impactful stuff that permanently removes carbon from our atmosphere”.
The team of byterocket reviewed and audited the above smart contracts in the course of this audit. We started on the 16th of May and finished on the 29th of May 2022.
byterocket gained access to the code via a private GitHub repository. We based the audit on the main branch’s state on May 10th, 2022 (commit hash f938db47801436d31e709723176ec29165756f03). The updated version was provided to us via multiple new commits to the repository, addressing our findings. The last and most recent commit hash that we audited is 0fb352c8e43a8edbebd7b2e0f42fff7d77a0c45c from June 3rd, 2022.
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 22 bugs or flaws, with 22 of them being fixed in a subsequent update. Prior to this, there have been 11 non-critical, 8 of low severity and 3 of medium severity findings. Our automated systems and review tools did not find any additional ones. Additionally, we found 6 gas improvements.
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 sufficiently documented.
Location: Settlement.sol - Line 62
Description:
While it is verified that the fromToken of the first order matches to toToken of the second order, it is not enforced that the toToken of the first order matches the fromToken of the second order. This might lead to settlements of orders that only partly match in terms of their tokens.
These are the affected lines of code:
Recommendation:
Consider also checking whether order1.toToken == order2.fromToken to ensure that both orders match on both tokens.
Update on 5th of June 2022:
The developers have added the additional require statement to ensure what we recommended.
Location: Settlement.sol - Line 128 - 138
Description:
There are four ERC20 transfers in the _settle() function that could have a severe impact if they were used as an entry point for an attack. ERC20 tokens should always be handled via a safe library if possible.
These are the affected lines of code:
Recommendation:
Consider making use of an established library like SafeERC20 to ensure the safety of the transfers.
Update on 5th of June 2022:
The SafeERC20 library is now being used and all of the occurrences of transfers have been updated accordingly.
Location: SettlementNCTShortcut.sol - Line 78 - 79
Description:
In the NCT shortcut contracts _checkShortcutValid() function it is assumed that the orders toToken is NCT/TCO2. However, this should be checked on whether it is actually the case and not just assumed.
These are the affected lines of code:
Recommendation:
Consider adding a further check to ensure that the order actually had the toToken set to the right token.
Update on 5th of June 2022:
The corresponding function does now return a false flag in case of the toToken is not NCT/TCO2.
Location: Global - Line 1
Description:
The Solidity version is not explicitly defined. Additionally, the specified version is outdated.
These are the affected lines of code:
Recommendation:
Consider setting the compiler version to one of the latest releases without directly allowing all of the subsequent releases to be valid without you checking it.
Update on 5th of June 2022:
The compiler version has been fixed to the latest version at this time, 0.8.14.
Location: Settlement.sol - Line 134
Description:
Due to the use of percentage values that are in the 1e18 realm instead of 1e4, the risk for an overflow is unnecessarily increased. This does not seem worth it to us.
These are the affected lines of code:
Recommendation:
Consider rethinking the way you handle percentage values. It is the implicit standard way to use basis points (10,000), which accounts for percentage values down to 0.01%, which is usually enough. Only make use of greater number if it is required for accuracy.
Update on 5th of June 2022:
The developers have adopted our recommendation and are now working with basis points.
Location: Settlement.sol - Line 137
Description:
With the fee being sent to the contract itself, the fee can only be obtained via a function that transfers these accrued tokens to an external address. However, there is no such function, rendering the accrued fees to be stuck forever.
These are the affected lines of code:
Recommendation:
Consider adding a functionality to withdraw any accrued fees.
Update on 5th of June 2022:
The contract now has an owner - making use of the Ownable library by OpenZeppelin - which is the receiver of the fees.
Location: SettlementNCTShortcut.sol - Line 29 + 30
Description:
It is usually not within the best practice to hardcode values inline outside of early development stages. As the contract can be deployed to multiple networks, it would be bad to have an unnoticed hardcoded value in the contract. This should be in a variable or - which would be even better - be a parameter for the constructor to offer maximum flexibility.
These are the affected lines of code:
Recommendation:
Consider adding the address to the constructor or to a global variable.
Update on 5th of June 2022:
The hardcoded addresses have been removed from the contract and have been replaced with constructor parameters according to our recommendation.
Location: SettlementNCTShortcut.sol - Line 92
Description:
While all of the paths besides this one include a return, one doesn’t. This does not look like it has been done on purpose, hence we assume that it is missing.
These are the affected lines of code:
Recommendation:
Consider adding the correct return value (which would be the boolean false) to this branch as well.
Update on 5th of June 2022:
The corresponding path now returns a false if it is reached, which conforms to our recommendation.
Location: SettlementNCTShortcut.sol - Line 145 + 155 - 156
Description:
Analogous to finding (M.2) but with a lesser impact, this is another occurrence where ERC20 interactions are not handled safely.
These are the affected lines of code:
Recommendation:
Consider making use of an established library like SafeERC20 to ensure the safety of the transfers while addressing (M.2).
Update on 5th of June 2022:
Together with the fix of M.2., the team has properly addressed the outlined issue.
Location: SettlementNCTShortcut.sol - Line 30 + 156
Description:
The approval is already being set to max-uint256 in the constructor, while it is being set here again, essentially setting it to the amount after the transfer each time.
These are the affected lines of code:
Recommendation:
Consider removing one of the approvals or making use of OpenZeppelins safe- IncreaseApproval to be on the safe side.
Additional note: NCT does not interpret max-uint256 as infinity but instead reduces the amount with each transaction, which is a bit special given todays standards. We just want the developers to keep this in mind.
Update on 5th of June 2022:
The developers have removed the first approval in the constructor, which was one of our recommendations.
Location: SettlementNCTShortcut.sol - Line 162
Description:
The deadline of the token swap is extended by thirty seconds during the call, which is only relevant for externally owned accounts, as the transaction has to be included in time, which can take time. As this call, however, is only executed during a transactions, the deadlines extension is meaningless.
These are the affected lines of code:
Recommendation:
Consider removing the extension of the deadline, as it is unnecessary and just costs gas due to the addition.
Update on 5th of June 2022:
The developers have removed the 30 second extension and are now using the unmodified timestamp.
Location: Settlement.sol - Line 37
Description:
The chain id of the underlying EVM chain can easily and provably be correctly obtained via the block.chain.id command. However, it is being passed as a parameter here, which is just unnecessary in this case.
These are the affected lines of code:
Recommendation:
Consider making use of the Solidity specific command of block.chain.id instead of manually setting the chain id.
Update on 5th of June 2022:
The developers have adapted the constructor to work with a chainid parameter, just like we recommended.
Location: Settlement.sol - Line 37
Description:
As this accepted pattern prevents contracts from calling the contract, it subsequently also protects from reentrancy attacks, which is not documented as it should be.
These are the affected lines of code:
Recommendation:
Consider adding some documentation to this specific line, indicating that it also adds reentrancy protection.
Additional note: This require statement is used multiple times, it might make sense to export it into a modifier like onlyEOA(msg.sender).
Update on 5th of June 2022:
The developers have added a line of documentation above to indicate the purpose of this statement.
Location: Settlement.sol - Line 62
Description:
There is a typo in the error message of the require statement, where “eachtother” should be “eachother”.
These are the affected lines of code:
Recommendation:
Consider fixing the typo to ensure a clear understanding as well as front-end handling of each error message.
Update on 5th of June 2022:
The typo has been fixed.
Location: Settlement.sol - Line 119
Description:
Considering the note in the comment as well as the naming of the other variable O_s, it seems that the variable I_ should actually be called I_s.
These are the affected lines of code:
Recommendation:
Consider changing the name of the variable I_ to I_s.
Update on 5th of June 2022:
The developers acknowledged the finding but have not changed the naming, which is fine as this does not have any implications on the contract security.
Location: Settlement.sol - Line 149 - 150 + SettlementNCTShortcut - Line 188
Description:
There are multiple occurrences where a simple addition of a value is handled in the “complex” way instead of simply making use of the += operator. Using the correct operator would highly increase readability as well as ensure that no copy-paste errors occur.
This is one of the affected lines of code:
Recommendation:
Consider making use of the += operator.
Update on 5th of June 2022:
The corresponding additions have been adapted according to our recommendations.
Location: SettlementNCTShortcut - Line 30
Description:
It is not the best practice anymore to calculate the maximum value of a type manually, as Solidity offers a simpler as well as cheaper way to handle it via type(uint256).max.
These are the affected lines of code:
Recommendation:
Consider making use of the type(uint256).max operation to adhere to the current best practice.
Update on 5th of June 2022:
The corresponding line of code has been removed.
Location: SettlementNCTShortcut - Line 108
Description:
There is a duplication of code with the _settle() and batchFill() function, which could be prevented to potentially decrease the contract size and increase readability.
Recommendation:
Consider exporting the duplicate code into an internal function. This is just a suggestion and does not need to be addressed.
Update on 5th of June 2022:
The developers have moved a lot of the functionality into internal functions like we recommended.
Location: SettlementNCTShortcut - Line 136
Description:
There are multiple occurrences of different styles of error messages in this project, which is not part of the best practice.
These are the affected lines of code:
Recommendation:
Consider unifying the style of the error messages.
Update on 5th of June 2022:
The developers acknowledged the finding but have not changed the error message, which is fine as this does not have any implications on the contract security.
Location: SettlementNCTShortcut - Line 190
Description:
The error message for this require statement is not valid, as it can also be the small order, not just the big one.
These are the affected lines of code:
Recommendation:
Consider using a more generalized error message like “invalid signature for order”.
Update on 5th of June 2022:
The developers acknowledged the finding but have not changed the error message, which is fine as this does not have any implications on the contract security.
Location: libraries/Orders.sol - Line 7
Description:
As most users never verify a given hash to equal the keccak computation, it is not within the best practice to hardcode hash values.
These are the affected lines of code:
Recommendation:
Consider calculating the hash as described in your comment, instead of hardcoding the hash. There are no gas savings in hardcoding, as the compiler only calculates the keccak hash once during deployment for constants.
Update on 5th of June 2022:
The developers acknowledged the finding but have not changed the hash being hardcoded, which is fine as this does not have any direct implications on the contract security.
Location: libraries/Orders.sol - Line 55
Description:
With the current logic only enforcing that there is a deadline, it is not ensured that the deadline is actually in the future. This allows users to create orders that are already outdated.
These are the affected lines of code:
Recommendation:
Consider replacing 0 with block.timestamp to only allow valid orders to be created.
Update on 5th of June 2022:
The developers have replaced the 0 with block.timestamp accordingly.
Location: Throughout the project
Description:
All of the for-loops in the project are using the standard way of declaring them, which is slightly inefficient gas-wise.
The following loops are not optimized:
This is an example of an unoptimized for-loop:
Recommendation:
Consider optimizing the for-loops in each contract to be more efficient in terms of its gas usage, including:
Location: Throughout the project
Description:
In require statements, it is cheaper to check for != 0 instead of > 0, if the unsigned integer is being validated to not be zero. Changing this will save some gas.
The affected variables are:
This is an example of an affected require statement check:
Recommendation:
Consider changing all of the require statements that involved a zero check for a unsigned integer to be more gas efficient, like in the example below:
Location: Throughout the project
Description:
In certain circumstances, it saves gas to cache variables that are read often, especially if they are stored in storage and not in memory.
The affected variables are:
Recommendation:
Consider caching the affected variables and reading from the cached version instead of doing costly read actions.
Location: Settlement.sol - Lines 89
Description:
It is not required to set isBigOrder1 to its default value false.
Recommendation:
Consider not setting isBigOrder1 to false, since it unnecessarily uses gas.
Location: Settlement.sol - Lines 147 + 150
Description:
As there is a cached variable with the hash already, make use of it instead of re-accessing the storage of the hash, which just costs more gas.
These are the affected lines of code:
Recommendation:
Consider using hashSmall instead of small.hash().
Location: Throughout the project
Description:
If a public function is not used within the contract itself, it is a good idea to set it to be external, as this saves gas.
The following functions can be declared as external:
Recommendation:
Consider using hashSmall instead of small.hash().
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 22 bugs or flaws, with 22 of them being fixed in a subsequent update. Prior to this, there have been 11 non-critical, 8 of low severity and 3 of medium severity findings. Our automated systems and review tools did not find any additional ones. Additionally, we found 6 gas improvements.
The protocol review and analysis did neither uncover any game-theoretical nature problems nor any other functions prone to abuse besides the ones that have been uncovered in our findings.
In general, there are some improvements that can be made, but we are 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.
