|
Audited By: Angelos Apostolidis angelos.apostolidis@ourovoros.io George Delkos george.delkos@ourovoros.io |
| Project Name | Immunefi - Vault Safe |
| Website | Immunefi |
| Description | Bug Bounty Vaults |
| Platform | Ethereum; Solidity, Yul |
| Codebase | GitHub Repository |
| Commits |
4055bbfe9b9d0e1e157b752898cf815e18cfc2d0 |
| Delivery Date | February 14th, 2023 |
| Method of Audit | Static Analysis, Manual Review |
| Total Issues | 4 |
| Total Major | 0 |
| Total Minor | 2 |
| Total Informational | 2 |
| Contract | Location |
|---|---|
| src/Withdrawable.sol | https://github.com/immunefi-team/vaults/tree/4055bbfe9b9d0e1e157b752898cf815e18cfc2d0/src/Withdrawable.sol |
| src/Splitter.sol | https://github.com/immunefi-team/vaults/tree/4055bbfe9b9d0e1e157b752898cf815e18cfc2d0/src/Splitter.sol |
| ID | Title | Type | Severity |
| F-1 | Ambiguous event definition | Gas Optimization | informational |
| F-2 | Usage of `transfer()` for sending Ether | Volatile Code | minor |
| F-3 | Inexistent input sanitization | Volatile Code | minor |
| F-4 | Redundant use of `virtual` | Coding Style | informational |
| Type | Severity | Location |
|---|---|---|
| Gas Optimization | informational | Withdrawable L17, L33 |
The LogWithdraw event includes data unrelated to the ERC-20 token standard, i.e. a tokenId, leading to static data
logging during the said event's emission.
We advise to remove the tokenId parameter from the LogWithdraw event.
The team has acknowledged this exhibit but decided to include its alleviation on the next iteration of the code.
| Type | Severity | Location |
|---|---|---|
| Volatile Code | minor | Withdrawable L28 |
After EIP-1884 was included in the Istanbul hard fork, it is not recommended
to use .transfer() for transferring ether as these functions have a hard-coded value for gas costs making them
obsolete as they are forwarding a fixed amount of gas, specifically 2300. This can cause issues in case the linked
statements are meant to be able to transfer funds to other contracts instead of EOAs.
We advise that the linked .transfer() calls are substituted with the utilization of
the sendValue() function
from the Address.sol implementation of OpenZeppelin either by directly importing the library or copying the linked
code.
The team has acknowledged this exhibit but decided to include its alleviation on the next iteration of the code. Also, the team has prepared a process to overcome this risk by transferring ownership to an EOA to recover the funds.
| Type | Severity | Location |
|---|---|---|
| Volatile Code | minor | Splitter L45, L76, L85, 109 |
The linked code expressions fail to check the address-based values against the zero address.
We advise to add require statements, checking the aforementioned values against the zero address.
The team has acknowledged this exhibit but decided to include its alleviation on the next iteration of the code. Also, the team has communicated the risks for using specific values as arguments.
| Type | Severity | Location |
|---|---|---|
| Coding Style | informational | Splitter L135 |
The linked function contains the virtual keyword without the intention of being extended.
We advise to remove the virtual keyword from the linked function.
The team has acknowledged this exhibit but decided to include its alleviation on the next iteration of the code, as it relates to code maintainability and does not affect the users.
Reports made by Ourovoros are not to be considered as a recommendation or approval of any particular project or team. Security reviews made by Ourovoros for any project or team are not to be taken as a depiction of the value of the “product” or “asset” that is being reviewed.
Ourovoros reports are not to be considered as a guarantee of the bug-free nature of the technology analyzed and should not be used as an investment decision with any particular project. They represent an extensive auditing process intending to help our customers increase the quality of their code while reducing the high level of risk presented by cryptographic tokens and blockchain technology.
Each company and individual is responsible for their own due diligence and continuous security. Our goal is to help reduce the attack parameters and the high level of variance associated with utilizing new and consistently changing technologies, and in no way claim any guarantee of security or functionality of the technology we agree to analyze.