Hi everyone, I’m Samrat Gupta, also known as Sm4rty, an independent security researcher and smart contract auditor. In this blog post, I’ll share my experience auditing with DetectBox, an awesome platform. I recently had the opportunity to audit three projects with DetectBox. In this post, I’ll share my experience and a few of my findings from one of the audits, Kunji Finance.
Let’s start:
Before starting out. Let me give a brief description of Detectbox.
About DetectBox:
DetectBox is an emerging auditing marketplace for auditors and Projects. Unlike traditional auditing firms that keep auditors’ identities hidden, DetectBox promotes transparency. It allows projects and protocols to choose auditors independently to evaluate their work. This inclusive process ensures that experts from different backgrounds can contribute to the auditing process. The platform also helps us learn and collaborate with fellow auditors.
Check out their official Website for more info.
Audits with Detect Box:
I recently had the opportunity to audit three projects with DetectBox:
- Etherverse Token: This was a basic ERC20 token with a presale contract. I didn’t find any major issues with this project, as there was nothing much interesting to look for. I found 1 medium issue along with a few low-severity issues and gas optimizations.
Check out the audit report here. - Kunji Finance: This is an asset management platform built on Arbitrum Chain. I found 6 medium-severity issues, as well as some low-severity issues and gas optimizations.
Check out the audit report here. - Escrow contract built on top of Rain Protocol: (cannot disclose more details until the audit is completed) This is the most challenging project so far, as the contract was written in Rainlang and I needed to learn the whole of Rain Protocol and Rainlang from scratch. I found 2 high-severity issues, 1 medium-severity, and 10+ low and gas optimizations.
I will be sharing my experience and a few of my findings from the Kunji Finance audit in this blog. I will write about the escrow contract on Rain Lang in the next blog.
Collaborations with Detect Warden:
Before moving further, let me give a brief description on the role of Detect Wardens in DetectBox audits.
Who are Detect Wardens?
Detect Wardens are security researchers appointed by DetectBox to monitor the progress of the audit, resolve discrepancies between the client and audit team, and perform audit mitigation. They play an important role in ensuring the quality and accuracy of the audit.
For the Kunji Finance audit, I had the opportunity to collaborate with Rohan Jha, who is a Detect Warden. Rohan is a skilled auditor with a deep understanding of smart contract security. It was a great opportunity to work with him.
Key Discoveries
In this blog, I will be sharing a few findings from a recent audit for Kunji Finance. Check here for the full report.
Now let’s look at some of the findings:
1. Usage of a Non-Upgradable version of OZ contracts in Upgradable Contracts.
The contract is designed as an upgradeable proxy contract. However, the current iteration of contracts imports the non-upgradable versions of SafeERC20, IERC20Metadata, and EnumerableSet from the OpenZeppelin library. An upgradable version of the contract ensures that the code is safe for upgradeability.
The use of non-upgradable versions of certain OpenZeppelin contracts within upgradeable contracts can lead to potential issues and inconsistencies.
import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";The project mitigated the issue by using the upgradable version of these imports.
import {SafeERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
import {EnumerableSetUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/structs/EnumerableSetUpgradeable.sol";2. Missing Access control in emergencyClose() function
In the UserVault.sol contract, there exists emergencyClose() function that lacks proper access control. The function visibility is marked as external, which means that any external user can call this function. Additionally, there are no validation checks in the function to verify that the caller is authorized to close the user’s position.
There may be times when a user wants to keep a position open for a longer period of time in order to realize a profit. An attacker could exploit this vulnerability to call the emergencyClose() function and close the user’s Uniswap/GMX position without their permission.
function emergencyClose() external { //@access control issue
address _traderWalletAddress = traderWalletAddress;
if (
ITradeWallet(_traderWalletAddress).lastRolloverTimestamp() +
emergencyPeriod >
block.Timestamp &&
isEmergencyOpen
) revert TooEarly();
bool isRequestFu1fi11ed = _closeUniswapPositions(_traderWalletAddress);
_closeGmxPositions(_traderWalletAddress);
if (!isRequestFu1fi11ed) {
currentSlippage += slippageStepPercent;
isEmergencyOpen = true;
} else {
currentSlippage = defaultSlippagePercent;
isEmergencyOpen = false;
}
}3. Chainlink Sequencer Uptime Check doesn’t function as intended
In the contract, the _checkSequencerUptimeFeed function was implemented to check for Chainlink Sequencer Uptime. However, the function was not implemented correctly.
function _checkSequencerUptimeFeed() private view {
address _sequencerUptimeFeed = sequencerUptimeFeed;
if (_sequencerUptimeFeed == address(0)) {
return;
}
(, int256 answer, uint256 startedAt, , ) = AggregatorV2V3Interface(
_sequencerUptimeFeed
).latestRoundData();
// Answer == 0: Sequencer is up
// Answer == 1: Sequencer is down
bool isSequencerUp = answer == 0;
if (!isSequencerUp) {
revert SequencerDown();
}
// Make sure the grace period has passed after the
// sequencer is back up.
uint256 timeSinceUp = block.timestamp - startedAt;
if (timeSinceUp <= _GRACE_PERIOD_TIME) {
revert GracePeriodNotOver();
}In the DynamicValuation contract, the _checkSequencerUptimeFeed() function is only implemented in the _getUSDValueOfAddress() function. This means that the sequencer uptime is not checked when fetching price feeds in the _getDataFeedAnswer() function. It leads to insufficient checks for if the sequencer is active. If the sequencer is down, then the price feeds returned by the _getDataFeedAnswer() function will be incorrect. This could lead to incorrect valuations of tokens and assets, which could have financial consequences for users of the DynamicValuation contract.
function _getDataFeedAnswer(
OracleData memory oracleData,
address token
) private view returns (uint2S6) {
if (oracleData.dataFeed == address(O)) {
revert leFocToken(token) ;
}
AggregatorV2V3Interface _dataFeed AggregatorV2V3Interface(
oracleData. dataFeed
);
(, int answer, uint2S6 updatedAt, ) = _dataFeed.latestRoundData();
if (answer <= 0) {
revert BadPrice();
}
if(block.timeStamp - updatedAt > oracleData.heartBeat){
revert TooOldPrice();
}
return uint256(answer);
}Apart from these, there were a few more medium, low, and Gas Optimization issues. You can have a look at the full audit report here.
Conclusion:
I would like to thank DetectBox for the opportunity to audit these projects. It was a great learning experience, and I’m glad that I could contribute to the security of the DeFi ecosystem.
