Prepared by:
HALBORN
Last Updated 09/11/2024
Date of Engagement by: July 8th, 2024 - July 26th, 2024
88% of all REPORTED Findings have been addressed
All findings
8
Critical
0
High
0
Medium
1
Low
1
Informational
6
APRO engaged Halborn to conduct a security assessment on their smart contracts revisions beginning on 07/08/2024 and ending on 07/26/2024. The security assessment was scoped to the smart contracts provided to the Halborn team.
The team at Halborn was provided 3 weeks for the engagement and assigned a full-time security engineer to evaluate the security of the smart contract.
The security engineer is a blockchain and smart-contract security expert with advanced penetration testing, smart-contract hacking, and deep knowledge of multiple blockchain protocols.
The purpose of this assessment is to:
Ensure that smart contract functions operate as intended.
Identify potential security issues with the smart contracts.
In summary, Halborn identified some security risks that were mostly addressed by the APRO team.
Halborn performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy regarding the scope of this assessment. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance code coverage and quickly identify items that do not follow the security best practices. The following phases and associated tools were used during the assessment:
Research into architecture and purpose.
Smart contract manual code review and walkthrough.
Graphing out functionality and contract logic/connectivity/functions. (solgraph)
Manual assessment of use and safety for the critical Solidity variables and functions in scope to identify any arithmetic related vulnerability classes.
Manual testing by custom scripts.
Scanning of solidity files for vulnerabilities, security hot-spots or bugs. (MythX)
Static Analysis of security for scoped contract, and imported functions. (Slither)
Testnet deployment. (Brownie, Anvil, Foundry)
| 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
1
Low
1
Informational
6
| Security analysis | Risk level | Remediation Date |
|---|---|---|
| latestTimestamp create a false sense of data freshness | Medium | Risk Accepted |
| Lack of L2 Sequencer Status Monitoring in Deployment | Low | Future Release |
| Inconsistent Solidity Pragma Versions | Informational | Solved - 08/06/2024 |
| Unused Function decodeReport in OffchainAggregator Contract | Informational | Solved - 08/06/2024 |
| Outdated Version of solidity | Informational | Acknowledged |
| Cache Array Length outside of loop | Informational | Acknowledged |
| Missing SPDX License Identifier | Informational | Solved - 08/06/2024 |
| Inconsistent Naming Convention in Token Interface | Informational | Solved - 08/06/2024 |
// Medium
The current implementation of the latestTimestamp , determineTimestamp and getTimestamp functions in the EACAggregatorCombine contract can mislead users about the freshness of the data if one of the two aggregators has not updated recently.
By returning the latest from the two latest timestamps aggregators, the function may suggest that the data is up-to-date even if one aggregator's data is stale.
function latestTimestamp() public view virtual override returns (uint256) {
uint256 timestampX = aggregatorX.latestTimestamp();
uint256 timestampY = aggregatorY.latestTimestamp();
return timestampX > timestampY ? timestampX : timestampY;
}
Similarly, in the determineTimestamp function, the updatedAt variable is assigned the newer timestamp of the two aggregators:
function determineTimestamp(
uint256 startedAtX,
uint256 startedAtY,
uint256 updatedAtX,
uint256 updatedAtY
) internal pure returns (uint256 startedAt, uint256 updatedAt) {
if (/* .. */ ) { /* ... */ }
else {
startedAt = startedAtX > startedAtY ? startedAtY : startedAtX;
//E @audit updatedAt is the newest of the two
updatedAt = updatedAtX > updatedAtY ? updatedAtX : updatedAtY;
}
}This issue can create a false sense of data freshness, potentially leading to erroneous decisions based on outdated information. Users might rely on data they believe is recent, but in reality, it could include stale data from one of the aggregators.
Test Steps :
Aggregator X is updated with data
Wait 2 days(can be a lot more)
Aggregator Y is updated with data
Data and latestTimestamp are retrieved using combine proxy
LatestTimestamp of AggregatorY is returned when Aggregator X data is 2 days old
function testLatestTimestampNotLatest() public {
// Prepare data for the report
(,,bytes32 configDigest) = offAX.latestConfigDetails();
uint32 epoch = 1;
uint8 round = 1;
bytes32 rawReportContext = bytes32(uint256(uint256(configDigest) << 24 | uint256(epoch) << 8 | uint256(round)));
bytes32 rawObservers = bytes32(0); // Example value
int192[] memory observations = new int192[](4);
observations[0] = 100e18;
observations[1] = 200e18;
observations[2] = 300e18;
observations[3] = 400e18;
bytes memory report = abi.encode(rawReportContext, rawObservers, observations);
// Generate signatures for the report
(bytes32[] memory rs, bytes32[] memory ss, bytes32 rawVs) = generateSignatures(report);
// Call transmit function
vm.startPrank(transmitter1);
offAX.transmit(report, rs, ss, rawVs);
vm.stopPrank();
vm.warp(block.timestamp + 2 days);
vm.startPrank(transmitter2);
offAY.transmit(report, rs, ss, rawVs);
vm.stopPrank();
int256 latestAnswerX = offAX.latestAnswer();
int256 latestAnswerY = offAY.latestAnswer();
// console.log("latestAnswerX = %s", uint256(latestAnswerX) / 1e18);
// console.log("latestAnswerY = %s", uint256(latestAnswerY) / 1e18);
// console.log("latestAnswerEAC = %s", uint256(eacACP.latestAnswer()) / 1e18);
console.log("Current Timestamp : %s",block.timestamp);
console.log("Latest Timestamp of EACAggregatorCombine : %s",eacACP.latestTimestamp());
console.log("Latest Timestamp of Aggregator X : %s",offAX.latestTimestamp());
console.log("Latest Timestamp of Aggregator Y : %s",offAY.latestTimestamp());
}As it can be seen in the result, latestTimestamp of AggregatorX is 2 days stale but data is returned like it was updated at the current timestamp
To ensure the accuracy and reliability of the reported data, it is recommended to modify the latestTimestamp function to return the oldest timestamp from the two aggregators or to compare it with a maximum staleness variable. This approach ensures that the data is only considered fresh if both aggregators have been updated recently.
RISK ACCEPTED : The APRO team accepted the risk, they said that if users want to have an idea of both data freshness they can request both aggregator latest timestamp
// Low
The audited system, a fork of Chainlink, is designed for deployment across multiple EVM-compatible chains, including Layer 2 (L2) solutions and sidechains. These chains include Scroll, Linea, ZKLink Nova, BSC, and others that are not L2s or L2s. A critical omission has been identified in the implementation: the absence of L2 sequencer feeds contracts on these chains for Apro Contract.
L2 solutions and some sidechains utilize sequencers to batch and process transactions. The operational status of these sequencers is crucial for the integrity and fairness of the oracle service. Without sequencer status monitoring, the system lacks the capability to detect and respond to sequencer downtime or unavailability.
Chainlink's standard implementation includes sequencer feeds to provide real-time status updates of L2 sequencers. These feeds allow oracle consumers to determine whether the L2 network is fully operational and accessible to all users.
The oracle may continue to report prices without accounting for the inaccessibility of the L2 network to most users, potentially leading to mispriced assets or unfair liquidations.
To address this issue, one of the following measures is recommended:
Implement L2 Sequencer Feeds: Deploy sequencer status feed contracts on all supported L2 and sidechain networks, following Chainlink's established patterns.
Integrate Sequencer Status Checks: Modify the oracle contracts to incorporate checks against the sequencer status feeds before providing price data.
PENDING: The APRO team will solve the issue by deploying a L2 Sequencer Feed on Scroll Rollup
// Informational
Apro contracts use multiple versions of the Solidity compiler pragma, specifically 0.7.6 and ^0.7.0. This inconsistency can lead to compatibility issues, as different pragma versions may have varying behavior and features. The following files use 0.7.6:
AccessControlledOffchainAggregator.sol
OffchainAggregator.sol
OffchainAggregatorBilling.sol
Owned.sol
SimpleReadAccessController.sol
SimpleWriteAccessController.sol
The following files use ^0.7.0:
AccessControllerInterface.sol
AggregatorInterface.sol
AggregatorV2V3Interface.sol
AggregatorV3Interface.sol
AggregatorValidatorInterface.sol
LinkTokenInterface.sol
TypeAndVersionInterface.sol
AproTokenInterface.sol
Additionally, the usage of solc-0.7.6 is flagged as not recommended for deployment. It is crucial to use consistent and up-to-date Solidity versions to ensure contract security and compatibility.
It is recommended to use a consistent and up-to-date pragma version across all contracts. It is recommended to upgrade to the latest stable Solidity version (e.g., 0.8.16 or higher) to benefit from the latest security features and improvements.
SOLVED : A consistent solidity pragma is now used.
// Informational
The function decodeReport in the OffchainAggregator contract is identified as unused and effectively dead code because of the internal modifier. This function is designed to decode a report from a byte array into its constituent components: rawReportContext, rawObservers, and observations. The code is as follows:
function decodeReport(bytes memory _report) internal pure
returns (
bytes32 rawReportContext,
bytes32 rawObservers,
int192[] memory observations
)
{
(rawReportContext, rawObservers, observations) = abi.decode(_report,(bytes32, bytes32, int192[]));
}The decodeReport function is intended for internal use but is never called within the contract, making it dead code.
It is recommended to remove the decodeReport function from the contract to streamline the codebase and eliminate unnecessary code.
SOLVED : The APRO team removed the unused function.
// Informational
During the security assessment, it was identified that the project uses an outdated version of Solidity (pragma solidity ^0.7.0;). This version was released on July 28, 2020, and has several known bugs that could potentially impact the security and functionality of the smart contracts.
The known bugs in Solidity version 0.7.0 include:
These bugs can lead to unexpected behavior or exposure to known security vulnerabilities.
It is strongly recommended to upgrade to the latest stable version of Solidity to mitigate the risks associated with these known bugs. By upgrading, the project can benefit from the latest security patches, improvements, and new features introduced in the newer versions of Solidity.
ACKNOWLEDGED: The APRO team acknowledged the finding.
// Informational
When the length of an array is not cached outside of a loop, the Solidity compiler reads the length of the array during each iteration. For storage arrays, this results in an extra sload operation (100 additional gas for each iteration except the first). For memory arrays, this results in an extra mload operation (3 additional gas for each iteration except the first).
For example :
for (uint i = 0; i < _signers.length; i++) { // add new signer/transmitter addresses
require(
s_oracles[_signers[i]].role == Role.Unset,
"repeated signer address"
);
s_oracles[_signers[i]] = Oracle(uint8(i), Role.Signer);
require(s_payees[_transmitters[i]] != address(0), "payee must be set");
require(
s_oracles[_transmitters[i]].role == Role.Unset,
"repeated transmitter address"
);
s_oracles[_transmitters[i]] = Oracle(uint8(i), Role.Transmitter);
s_signers.push(_signers[i]);
s_transmitters.push(_transmitters[i]);
}It is recommended to cache the length of the array in a local variable outside the loop to optimize gas usage. This reduces the number of read operations required during each iteration of the loop.
ACKNOWLEDGED: The APRO team acknowledged the finding.
// Informational
The EACAggregatorProxy.sol does not include an SPDX license identifier on the first line. The SPDX identifier is a recommended standard that specifies the software license governing the code, ensuring clarity and compliance with legal requirements.
pragma solidity 0.7.6;
/**
* @title The Owned contract
* @notice A contract with helpers for basic contract ownership.
*/
contract Owned {
...
}The absence of an SPDX license identifier can lead to potential legal ambiguities. Without a clear license declaration, users and developers may be uncertain about the usage, modification, and distribution rights of the contract. This can hinder collaboration and adoption, and may lead to legal complications if the code is used or modified without proper licensing information.
To adhere to best practices and legal standards, it is recommended to include an SPDX license identifier at the beginning of the contract file. This provides clear information about the licensing terms under which the code is distributed.
By adding the SPDX license identifier, you ensure that the licensing information is explicitly communicated, reducing legal risks and promoting better code practices.
SOLVED: The APRO team added the SPDX License Identifier to solve the issue.
// Informational
The smart contract under review contains an interface named LinkTokenInterface in file AproTokenInterface.sol which is a direct copy from the Chainlink project. However, this project is named APRO, and the interface name has not been updated to reflect this fact. The current implementation is as follows:
// SPDX-License-Identifier: MIT
pragma solidity ^0.7.0;
interface LinkTokenInterface {
// ... (interface functions)
}This naming inconsistency is present throughout the interface, including comments that directly reference Chainlink:
// The client constructs and makes a request to a known Chainlink oracle through the transferAndCall function, implemented by the LINK tokenTo address this issue, the following changes are recommended:
Rename the interface to reflect the APRO project
Update all references to Chainlink in comments and documentation within the interface
SOLVED : Naming conventions has been modified.
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.
All issues identified by Slither were proved to be false positives or have been added to the issue list in this report.
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