Prepared by:
HALBORN
Last Updated 06/26/2024
Date of Engagement by: May 8th, 2024 - May 10th, 2024
100% of all REPORTED Findings have been addressed
All findings
10
Critical
0
High
0
Medium
0
Low
4
Informational
6
Taurus Labs engaged Halborn to conduct a security assessment on their smart contracts beginning on 05-08-2024 and ending on 05-10-2024. The security assessment was scoped to the smart contracts provided in the https://github.com/tauruslabs/merkle-swapbridge/tree/main/src GitHub repository. Commit hashes and further details can be found in the Scope section of this report.
Halborn was provided 3 days for the engagement and assigned 1 full-time security engineer to review the security of the smart contracts in scope. The engineer is a blockchain and smart contract security expert with advanced penetration testing and smart contract hacking skills, and deep knowledge of multiple blockchain protocols.
The purpose of the assessment is to:
Identify potential security issues within the smart contracts.
Ensure that smart contract functionality operates as intended.
In summary, Halborn identified some improvements to reduce the likelihood and impact of risks, which were mostly addressed by the Taurus Labs team. The main identified issues were:
Operational reliance on external addresses.
Token approvals enabled during contract pause.
Centralization risk due to privileged access from owner.
Halborn performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of this assessment. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of the contracts' solidity code and can quickly identify items that do not follow security best practices. The following phases and associated tools were used throughout the term of the assessment:
Research into architecture and purpose.
Smart contract manual code review and walk-through.
Manual assessment of use and safety for the critical Solidity variables and functions in scope to identify any arithmetic-related vulnerability classes.
Local testing with custom scripts (Foundry).
Fork testing against main networks (Foundry).
Static analysis of security for scoped contract, and imported functions.
External libraries and financial-related attacks.
New features/implementations after/within the remediation commit IDs.
| EXPLOITABILIY METRIC () | METRIC VALUE | NUMERICAL VALUE |
|---|---|---|
| Attack Origin (AO) | Arbitrary (AO:A) Specific (AO:S) | 1 0.2 |
| Attack Cost (AC) | Low (AC:L) Medium (AC:M) High (AC:H) | 1 0.67 0.33 |
| Attack Complexity (AX) | Low (AX:L) Medium (AX:M) High (AX:H) | 1 0.67 0.33 |
| IMPACT METRIC () | METRIC VALUE | NUMERICAL VALUE |
|---|---|---|
| Confidentiality (C) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
| Integrity (I) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
| Availability (A) | None (A:N) Low (A:L) Medium (A:M) High (A:H) Critical (A:C) | 0 0.25 0.5 0.75 1 |
| Deposit (D) | None (D:N) Low (D:L) Medium (D:M) High (D:H) Critical (D:C) | 0 0.25 0.5 0.75 1 |
| Yield (Y) | None (Y:N) Low (Y:L) Medium (Y:M) High (Y:H) Critical (Y:C) | 0 0.25 0.5 0.75 1 |
| SEVERITY COEFFICIENT () | COEFFICIENT VALUE | NUMERICAL VALUE |
|---|---|---|
| Reversibility () | None (R:N) Partial (R:P) Full (R:F) | 1 0.5 0.25 |
| Scope () | Changed (S:C) Unchanged (S:U) | 1.25 1 |
| Severity | Score Value Range |
|---|---|
| Critical | 9 - 10 |
| High | 7 - 8.9 |
| Medium | 4.5 - 6.9 |
| Low | 2 - 4.4 |
| Informational | 0 - 1.9 |
Critical
0
High
0
Medium
0
Low
4
Informational
6
| Security analysis | Risk level | Remediation Date |
|---|---|---|
| Token approvals enabled during contract pause | Low | Solved - 05/10/2024 |
| Missing return value verification after swaps | Low | Risk Accepted |
| Centralization risk due to privileged access from Owner | Low | Risk Accepted |
| Single step ownership transfer risks DOS for privileged functions | Low | Solved - 05/10/2024 |
| Operational reliance on external addresses | Informational | Acknowledged |
| Incompatibility risk for EVM versions in different chains | Informational | Solved - 05/10/2024 |
| Unlocked pragma compiler | Informational | Solved - 05/10/2024 |
| Use of revert strings over custom errors increases gas costs | Informational | Solved - 05/10/2024 |
| Missing state variables visibility modifiers | Informational | Solved - 05/10/2024 |
| Missing events | Informational | Solved - 05/10/2024 |
// Low
The SwapBridge contract inherits from OpenZeppelins's Pausable contract and allows for control of function execution via the whenNotPaused modifier. However, the approveMax() function is not protected by this modifier. Consequently, token approvals can occur even when the contract is in a paused state, while the swapAndSendToAptos() and sendToAptos() functions, which are intended to be called after approval, would not be able to be called, interrupting the swap process.
Protect the approveMax() function with the whenNotPaused modifier to prevent the contract from being able to approve tokens while the contract is paused.
SOLVED: The Taurus Labs team has addressed the finding in commit b796692 by following the mentioned recommendation.
// Low
In the swapAndSendToAptos() function, the swapTarget address is utilized to execute the swap through OpenZeppelin’s Address library's functionCall() method, which returns a bytes value upon execution. However, the swapAndSendToAptos() function currently lacks verification of this return value from the swapTarget function call. This lack of verification could lead to unanticipated outcomes if the function call returns a value that is expected to be verified.
Handle the return value of the function call, especially if it is expected to contain critical data or if there is uncertainty about its content.
RISK ACCEPTED: The Taurus Labs team made a business decision to accept the risk of this finding and not alter the contracts, stating:
The target function of the invocation is one of the swap functions defined in the IParaswap interface. Checking the return value isn’t very practical since different swap functions in IParaswap have different return signatures. Also the value itself isn’t very useful for verification purpose and we can whether the swap has been executed correctly, which we verify by comparing the toToken balance and toAmount after the function call.
// Low
The SwapBridge contract inherits from OpenZeppelin's Ownable contract and declares the owner at deployment. This owner address is used to perform administrative actions on the contract, such as token withdrawals and pausing/unpausing contract functions.
This centralization of control over the contract may pose is a risk to the protocol, as this architecture may put the assets of users who have engaged with the protocol in a vulnerable position, exposing them to the potential threat of a rug pull. By centralizing control over the stored funds, the contract compromises on decentralization principles and elevates the risk where user assets could be abruptly withdrawn by the owner, leaving users with no recourse.
Several remedial strategies can be employed, including but not limited to: implementing role-based access control, creating low-privileged roles to execute daily tasks in the protocol to limit the impact of possible compromise, transitioning control to a Multi-Signature wallet setup, establishing community-driven governance for decision-making on fund management, integrating time locks and/or setting withdrawal limits to prevent abrupt fund depletion.
RISK ACCEPTED: The Taurus Labs team made a business decision to accept the risk of this finding and not alter the contracts, stating:
The contract is purposefully non-upgradeable in order to minimize centralization risk; the Owner has no power to affect change in the behavior of the contract, namely swapping and bridging.
// Low
The SwapBridge contract inherits from OpenZeppelin's Ownable contract and allows for single-step ownership transfer via the transferOwnership() function. Regarding this, it is crucial that the address to which ownership is transferred is verified to be active and willing to assume ownership responsibilities. Otherwise, the contract could be locked in a situation where it is no longer possible to make administrative changes to it.
Additionally, in the Ownable contract, the renounceOwnership() function allows to renounce to the owner permission. Renouncing ownership before transferring it would result in the contract having no owner, eliminating the ability to call privileged functions.
Consider using OpenZeppelin's Ownable2Step contract over the Ownable contract, or implementing similar two-step ownership transfer logic into the contract.
Additionally, it is recommended that the owner cannot call the renounceOwnership() function without first transferring ownership to another address. In addition, if a multi-signature wallet is used, the call to the renounceOwnership() function should be confirmed by most signers.
SOLVED: The Taurus Labs team has addressed the finding in commit b796692 by following the mentioned recommendation.
// Informational
The SwapBridge contract relies heavily on the tokenBridge and swapTarget addresses, which are established at contract initialization, to operate effectively. The tokenBridge address plays a critical role in quoting fees for swaps and facilitating the transfer of tokens to the Aptos chain. Meanwhile, the swapTarget address is core for executing swaps within the swapAndSendToAptos() function.
This reliance on external addresses underscores the necessity of ensuring these addresses are secure and trusted, as they have substantial control over critical functions of the contract.
Ensure the integrity and security of these addresses to maintain the contract's functionality and safeguard against potential unexpected vulnerabilities.
ACKNOWLEDGED: The Taurus Labs team made a business decision to acknowledge this finding and not alter the contracts, stating:
To minimize the attack surface, we will ensure the users of SwapBridge contract to approve only the minimal allowance required for each instance a user uses the contract. Moreover, the contract shouldn’t hold any surplus token holdings with normal usage (rescue function is there only to rescue any balance that someone might mistakenly send to the contract).
// Informational
From Solidity versions 0.8.20 through 0.8.24, the default target EVM version is set to Shanghai, which results in the generation of bytecode that includes PUSH0 opcodes. Starting with version 0.8.25, the default EVM version shifts to Cancun, introducing new opcodes for transient storage, TSTORE and TLOAD.
In this aspect, it is crucial to select the appropriate EVM version when it's intended to deploy contracts on networks other than the Ethereum mainnet, such as L2 chains, which may not support these opcodes. Failure to do so could lead to unsuccessful contract deployments or transaction execution issues.
Make sure to specify the target EVM version when using Solidity versions from 0.8.20 and above, especially if deploying to chains that may not support newly introduced opcodes. Additionally, it is crucial to stay informed about the opcode support of different chains to ensure smooth deployment and compatibility.
SOLVED: The Taurus Labs team has addressed the finding in commit b796692 by following the mentioned recommendation and setting a specific EVM version for compilation and deployment. Further testing is required to ensure cross-chain compatibility.
// Informational
The current contract in scope has an unlocked compiler ^0.8.21. To ensure stability, it's important that contracts be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.
Lock the pragma version to the same version used during development and testing.
SOLVED: The Taurus Labs team has addressed the finding in commit b796692 by following the mentioned recommendation.
// Informational
In Solidity development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.
The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.
Consider replacing all revert strings with custom errors. For more reference, see here.
SOLVED: The Taurus Labs team has addressed the finding in commit b796692 by following the mentioned recommendation.
// Informational
The state variables declared in the SwapBridge.sol contract are not explicitly declared with any visibility modifiers. By default, the Solidity compiler will assume any state variable declared in the contract as internal.
It is considered best practice to explicitly specify visibility to enhance clarity and prevent ambiguity. Clearly labeling the visibility of all variables and functions will help in maintaining clear and understandable code.
Explicitly define the visibility for each state variable within the contract.
SOLVED: The Taurus Labs team has addressed the finding in commit b796692 by following the mentioned recommendation
// Informational
In Solidity development, emitting events is a recommended practice when state-changing functions are invoked. Currently, the SwapBridge contract lacks emission of events, besides the ones emitted during calls with third-party dependencies. This absence may hamper effective state tracking in off-chain monitoring systems.
Consider declaring and emitting events in the contract to enhance transparency and facilitate off-chain monitoring.
SOLVED: The Taurus Labs team has addressed the finding in commit b796692 by following the mentioned recommendation.
Halborn used automated testing techniques to enhance the coverage of certain areas of the smart contracts in scope. Among the tools used was Slither, a Solidity static analysis framework. After Halborn verified the smart contracts in the repository and was able to compile them correctly into their abis and binary format, Slither was run against the contracts. This tool can statically verify mathematical relationships between Solidity variables to detect invalid or inconsistent usage of the contracts' APIs across the entire code-base.
The security team assessed all findings identified by the Slither software, however, findings with related to external dependencies are not included in the below results for the sake of report readability.
The findings obtained as a result of the Slither scan were reviewed, and not included in the report because they were determined as false positives.
The original repository used the Foundry environment to develop and test the smart contracts against a local fork. All tests were executed successfully. Additionally, these tests were updated locally to generate additional fuzz tests that covered ~50,000 runs per test. These additional tests were run successfully.
Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed
