BlockApex (Auditor) was contracted by BullionFX (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which started on 20th Nov 2022.
Name: Rain Protocol |
Auditor: BlockApex |
Platform: EVM-based / Solidity |
Type of review: Manual Code Review | Automated Took Analysis |
Methods: Architecture Review | Functional Testing | Computer-Aided Verification | Manual Review |
Git repository/ Commit Hash: e2277e2253c60b04924f52b68e4ab6df4a68df6e |
White paper/ Documentation: Docs |
Document log: Initial Audit Completed: Nov 27th, 2022 Final Audit Completed: Nov 29th, 2022 |
The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | FT token API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Inheritance Ordering | Operation Trails & Event Generation |
Rain Protocol is a set of building blocks that enable your token economy including staking, vesting, emissions, escrow, order book, verification, sale, and membership. Rain VM uses EVM making it fully compatible with all the EVM chains. DEMI has integrated Rain protocol’s Staking contracts.
The system uses a design pattern called the ‘factory method’ to deploy homogenous contracts from the same parent & keeps a record of the child contracts that have been deployed by the factory. In this particular case, the system deploys a staking contract.
Staking contract is a modified version of a vault contract tokenized. It inherits all of the properties of the ERC-4626 contract which standardizes the interface for easily managing deposited tokens & their shares within the system. The contract also introduces a custom logic for maintaining its own internal ledger for share calculation through the checkpointing mechanism.
The codebase was audited using a filtered audit technique. A pair of two (2) auditors scanned the codebase in an iterative process spanning over a time of One (1) week.
Starting with the recon phase, a basic understanding was developed and the auditors worked on developing presumptions for the developed codebase and the relevant documentation/whitepaper. Furthermore, the audit moved on with the manual code reviews with the motive to find logical flaws in the codebase complemented with code optimizations, software, and security design patterns, code styles, best practices, and identifying false positives that were detected by automated analysis tools.
Our team performed a technique called “Filtered Audit”, where the contract was separately audited by four individuals. After a thorough and rigorous process of manual testing, an automated review was carried out using slither for static analysis. All the flags raised were manually reviewed and re-tested to identify the false positives.
Our team found:
# of issues | Severity of the risk |
0 | Critical Risk issue(s) |
0 | High Risk issue(s) |
0 | Medium |
1 | Low Risk issue(s) |
1 | Informatory issue(s) |
# | Findings | Risk | Status |
1 | Potential Loss of Precision | Low | Acknowledged |
2 | Inexplicable inclusion of unused library | Informational | Fixed |
No critical issues were found.
No High-risk issues were found.
No Medium-risk issues were found.
File: stake/Stake.sol#L15
Description:
In contracts/math/FixedPointMath.sol the function scaleN will lead to precision loss if a number is scaled down to 18.
There may arise a scenario in future development where a deposited token having a higher decimal (i.e > 18) may need to scale down to 18 decimals for logic consistency. The conversion either from or to 18 will lead to a loss in precision which may result in the dust amount being locked in the contract.
However, this function is not used in the current implementation of the staking contract to either scale up or down.
Recommendation:
Introduce a mechanism to manage the difference that was lost during the conversion. One way could be by storing the difference & using it to convert back.
DEMI Staking Audit - Report
Dev’s Response:
As noted staking contract does not use scaleN. Worth also noting that ERC4626 that the staking contract is based on dedicates a lot of the spec to rounding/dust handling, so if scaleN would be hypothetically used in the future it would still need to be 4626 compliant (which means always leaving dust from the underlying asset in the vault in the case of rounding issues) that's largely what the mulDiv is handling in openzeppelin's implementation and we wrap in the other fixed point math functions
As scaleN is a library contract it has no storage of its own so there's nowhere for it to save information about the lost precision directly, the best it could do is return two values, one representing the scaled value and one representing the lost precision. Currently, scaleN is only used in expressions in the interpreter as a provided opcode, so I'm not sure if it's something on the average expression author's radar to be worrying about or ready to handle (juggling 2 values on the stack to avoid some dust).
it's probably worth documenting all this though as it's worth pointing out as something to consider for anyone who does care about it.
DEMI Staking Audit - Report
Auditor’s Response
The auditors agree with the devs.
File: stake/Stake.sol#L15
Description:
The staking contract imports a library called FixedPointMath.sol that is not used within the scope of the contract.
Recommendation: Remove the unused import along with its using statement.
// import "../math/FixedPointMath.sol"; // using FixedPointMath for uint256; |
Alleviation: This issue is fixed.
The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices to date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report.
This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project.
Crypto assets/tokens are the results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service, or other assets. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.
Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer or any other areas beyond Solidity that could present security risks.
This audit cannot be considered as a sucient assessment regarding the utility and safety of the code, bug-free status, or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure the security of smart contracts.
Also see other Audit reports.
BlockApex (Auditor) was contracted by Dafi Protocol (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which took place from 16th Dec 2021 to 14th Jan 2022.
Name: Dafi Super Staking V2 |
Auditor: Moazzam Arif | Muhammad Jarir Uddin |
Platform: Ethereum/Solidity |
Type of review: Staking, Mathematics, Oracle feeds |
Methods: Architecture Review, Functional Testing, Computer-Aided Verification, Manual Review |
Git repository: https://github.com/DAFIProtocol/dDAFI/tree/testing |
White paper/ Documentation: https://docs.dafiprotocol.io/super-staking/overview-super-staking |
Document log: Initial Audit: 31st December 2021 (complete) |
Formal verification using property-based testing: 15th January 2022 |
Final Audit: 17th January 2022. |
The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/ vulnerabilities. Some specific checks are as follows:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | ERC20 API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Operation Trails & Event Generation |
A comprehensive explanation of Staking module V2 for the Dafi Protocol is present in the official documentation and can be viewed at: https://docs.dafiprotocol.io/super-staking/super-staking-v2
Dafi Protocol Super Staking V2 is an update to the V1 module released earlier this year (2021) in the month of July. Super Staking V2 claims to offer a more stable APY rate and enhanced distribution of dDAFI rewards by modifying the math behind reward calculation to rather depend only on accumulated amounts of reward every time demand factor changes to calculating the rewards of users as the sum of all rewards in the past adjusted to the latest demand factor.
Aside from the new reward formula, V2 also holds a couple of security improvements where demand factor is now enumerated using the latest price and is fortified by introduction of a delay i.e. a variance tolerance mechanism to ultimately prevent a sudden change in price. This new demand factor is supported by a TWAP calculation to make the price curve less steeper. Although strict monitoring is required, in cases of hackable oracle feeds, the protocol is able to recover from any kind of exploits by updating to an entirely new oracle service.
Codebase:
The system consists of 5 main smart contracts (namely: Staking Manager V2, Staking Database, Rebase Engine, Network Demand, Token Pool) and supplied with external data through 2 more contracts of Price Feeds and TVL Feeds.
Note: All of the contracts mentioned above contain onlyOwner modifiers to add, set and/or update configurations.
SHA-256 fingerprints of Contracts included:
Static-Analysis summary
Audit log
Manual Review: The audit launched with a recon phase where a manual code review was conducted to clarify the layers of understanding of the complexities and the general flow of the program. We started by reviewing the two main contracts against common solidity flaws. After the reconnaissance phase we wrote unit-test cases to ensure that the functions are performing their intended behavior. Then we began with the line-by-line manual code review.
Property Testing: From the reconnaissance, a handful of properties were also extracted and labeled as Invariants. In the following days of the audit procedure, the invariants were thoroughly tested against a setup flow of Dafi Staking V2 to ensure each of them held its proper definition.
The analysis indicates that some of the functionalities in the contracts audited are poorly-secured.
Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Mythril, MythX, and Slither. All the flags raised were manually reviewed and re-tested.
# of issues | Severity of the risk |
0 | Critical Risk issue(s) |
3 | High-Risk issue(s) |
0 | Medium Risk issue(s) |
1 | Low-Risk issue(s) |
1 | Informatory issue(s) |
Below is the summary of our findings from the complete audit. This includes any flags raised from the manual/automated code review, behavioral/scenario testing and the properties tested for formal verification.
S# | Properties/Findings | Risk/Impact | Status |
CR-1. | Oracle integrity | Critical | Passed |
HR-1. | MAX_DAFI always greater than totaldDafiDistributed | High | Failed |
HR-2. | RewardPool should not have balance if the Program is ended and all users have unstaked | High | Failed |
HR-3. | If a program has ended, users should not be able to stake | High | Failed |
LR-1. | Reward is claimable even after program has ended | Low | Passed |
LR-2. | Unit tests in the testing and main branch of repo at /test/v2.ts should pass | Low | Failed |
IR-1. | Memory optimization by using uint8 in place of true/ false | Informatory | - |
Status: Passed
In recent events, Oracles and external data sources have been manipulated to the adversary's benefit. We started off with our focus on the integrity of Oracles (Chainlink in this case) in order to make sure that the data imported into the contracts are not compromised.
The Chainlink oracle library method was tested against the following parameters of a standard audit technique.
function getThePrice() public view override returns (uint) {
(
,
int price,
,
,
) = priceFeed.latestRoundData();
return uint(price);
}
Suggestion: It is advised that a more tested and bonafide data source of TWAPs be considered to make Chainlink’s oracle integrity and reliability as the most optimal.
Status: Failed
If a program is marked as “ended” before the preset ending duration, that is to say, the elapsed time of the program is less than the program duration, users get revert transactions for staking into the ended program (as seen in the testnet event and confirmed over a fuzzing test scenario with the reason of MAX_DAFI being calculated less than total DAFI distributed till the ending timestamp). This property does not hold at fuzzed inputs;
function test_ConfirmMaxDafiOverflowFuzz(
uint256 md,
uint32 pd,
uint32 bn
) public {
SetupContracts caller = dapping.users(0).setupContract();
caller.wrapSEtStakingParams(1, md + 1, pd + 1);
hevm.warp(block.number + bn);
caller.wrapRebasePool();
caller.database().getPool();
uint256 MaxDafi = caller.rebaseEngine().MAX_DAFI();
uint256 TdDafidist = caller.rebaseEngine().totaldDAFIDistributed();
assertGt(MaxDafi, TdDafidist, 'maxDafi < tdDafiDist; program ended; dDafi overflows');
emit log_named_uint('value of MaxDafi', MaxDafi);
emit log_named_uint('value of TdDafiDist', TdDafiDist);
}
Suggestion: It is advised that program duration should always be maintained greater than the time elapsed by supplying checks that assure it.
if (database.isprogramEnded() && pool.lastUpdatedon > database.getProgramEndedAt()) {
return;
} else if (database.isProgramEnded() && pool.lastUpdatedon < database.getProgramEndedAt()) {
maxTimestampForcalc = database.getProgramEndedAt();
} else {
maxTimestampForCalc = block.timestamp;
}
Status: Failed
This property claims that the reward pool should be empty after the program has ended and that all the users have unstaked.
HR-2(a). “markProgramEnded()” by owner’s mistake can lock funds of the reward pool
In case of human error if the program is marked ended the reward pool retains an amount of tokens unclaimed against the percentage of tokens staked in first place.
function test_RewardpoolUnemptyAfterProgramEnded public {
SetupContracts caller = dapping.users().setupContract();
hevm.warp (block. timestamp + 5000);
caller.wrapSetStakingParams (1, 1000 ether, 12);
uint256 distPoolBalanceBefore = caller.stakingManager().distributionPool(). balance();
hevm.warp(block.timestamp + 99000);
caller.wrapstake ( 100);
caller.wrapMarkProgramEnded();
hevm warp (block timestamp + 99000);
caller.wrapunstake(100);
uint256 distPool BalanceAfter = caller.stakingManager().distributionPool(). balance();
assertGt(distPool BalanceBefore, distPoolBalanceAfter);
emit Log named uint('pool Before', distPoolBalanceBefore);
emit Log named uint('poolAfter', distPoolBalanceAfter);
}
Suggestion: It is advised to have a refund mechanism if program duration is ended. The refund amount of tokens should be equal to MAX_DAFI - (MDI*totalElapsedTime).
Status: Failed
In a simple scenario test, it was confirmed that Staking was allowed even if the program duration is completed and the program is marked as ended which in the understanding of the Auditor team is an (incomprehensible feature) and can be proved to be a loophole of the system in some scenarios.
Suggestion: The modifier stakeCheck should be supplied with some additional check to ensure monitoring of programs marked as ended not to allow staking.
function test_StakeAfterProgramEnded() public {
SetupContracts caller = dapping.users().setup Contract();
hevm.warp(block. timestamp + 500);
// _ms, _md, _pd
uint256 minStakeDays = 1;
uint256 maxDafi = 1000 ether;
uint32 progDuration = 12;
caller.wrapsetStakingParams (minStakeDays, maxDafi, progDuration);
// stakin/ unstaing multiple times for seeding
for (uint256 i = 0; i < 3; i++) {
hevm warp (block. timestamp + 90000);
caller.wrapStake ( 1000000000);
hevm warp(block. timestamp + 90000);
caller.wrapunstake ( 1000000000);
}
caller.wrapMarkProgramEnded();
// stakin/ unstaing multiple times for seeding
for (uint256 i = 0; i < 3; i++)
hevm.warp (block. timestamp + 90000);
caller.wrapStake ( 1000000000);
hevm.warp(block. timestamp + 90000);
caller.wrapUnstake ( 1000000000);
}
assertTrue(false);
}
Status: Passed
Regardless of the program being ended, users should be able to claim their rewards for their staked dafi tokens. This claim should not be bound by any type of time-related constraints. We checked whether this property would fail in any circumstance but it passed on all fuzzed inputs.
Status: Failed
Unit tests are critical in proving the developer's expected intention and behavior of the working code hence the set of tests not passing entirely and partially in the testing repository code is slightly questionable.
Following the best practices and the Solidity design patterns guide and since the client code uses a good number of state variables to manage the switches for staking, unstaking and alike. It is therefore suggested by the auditing team that uint8 type variables can be replaced by the developer in place of true/false on multiple occasions to minimize the memory usage and reduce the code size as an optimization.
The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report.
This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project.
Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.
Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.
This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.
BlockApex (Auditor) was contracted by VoirStudio (Client) for the purpose of conducting a Smart Contract Audit/ Code Review. This document presents the findings of our analysis which started from 25th Feb 2022.
Name: Unipilot-Farming-V2 |
Auditor: Kaif Ahmed | Muhammad Jarir Uddin |
Platform: Ethereum/Solidity |
Type of review: Manual Code Review | Automated Code Review |
Methods: Architecture Review, Functional Testing, Computer-Aided Verification, Manual Review |
Git repository: https://github.com/VoirStudio/unipilot-farming-v2 |
White paper/ Documentation: Not Provided |
Document log: Initial Audit: 4th March 2022 (Complete) |
Quality Control: 5th - 8th March 2022 |
Final Audit: 10th March 2022 (Complete) |
The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | ERC20 API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Operation Trails & Event Generation |
Unipilot yield farming incentivizes Liquidity Providers to earn $PILOT tokens by staking their Unipilot LP tokens on whitelisted pools.
Unipilot yield farming has 1 main smart contract; UnipilotFarm.sol: which allows Liquidity Providers to earn $PILOT token, an $ALT token, or both by staking their Unipilot LP tokens. UnipilotFarm linearly distributes the $PILOT according to rewardPerBlock and rewardMultiplier.
The codebase was audited in an iterative process. Fixes were applied on the way and updated contracts were examined for more bugs. We used a combination of static analysis tool (slither) and testing framework (hardhat) which indicated some of the critical bugs like reentrancy in the code. We also did manual reviews of the code to find logical bugs, code optimizations, solidity design patterns, code style and the bugs/ issues detected by automated tools.
The analysis indicates that the contracts audited are working properly.
Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Mythril, MythX, and Slither. All the flags raised were manually reviewed and re-tested.
# of issues | Severity of the risk |
3 | Critical Risk issue(s) |
0 | High-Risk issue(s) |
3 | Medium Risk issue(s) |
3 | Low-Risk issue(s) |
1 | Informatory issue(s) |
# | Findings | Risk | Status |
1. | First block reward is wrong in Only ALT and Dual Case. | Critical | Fixed |
2. | Consecutive change in reward type will disable the stakeLp() functionality. | Critical | Fixed |
3. | Miscalculation in pilot reward | Critical | Fixed |
4. | RewardToken validity should be check | Medium | Fixed |
5. | The UpdateRewardPerBlock() function will manipulate the last reward for every user. | Medium | Fixed |
6. | Should allow Parameters to send Zero to reopen farming | Medium | Fixed |
7. | RewardToken Zero Address check | Low | Fixed |
8. | Necessary checks in updateFarmingLimit() function | Low | Fixed |
9. | Emergency exit for users | Low | Fixed |
10. | Should update Multiplier and RewardType while calling init function. | Informatory | Fixed |
Description:
If the reward type is changed from only Pilot to only ALT or only Pilot to Dual, the code updates the lastBlockReward variable first for Pilot then for ALT, calculating block difference as 2 instead of 0. Hence, the user gets triple reward for just the first block.
else {
if (_rewardType == RewardType.Alt) {
altState.lastRewardBlock = blockNumber;
updateVaultState(_vault);
} else {
altState.lastRewardBlock = blockNumber;
}
}
}
emit RewardStatus(
_vault,
vaultInfo[_vault].reward,
vaultInfo[_vault].reward = _rewardType,
_altToken
);
Remedy:
When updating the reward type lastRewardBlock should be set to the current block number.
else {
if (_rewardType == RewardType.Alt) {
altState.lastRewardBlock = blockNumber;
vaultState.startBlock = blockNumber;
updateVaultState(_vault);
} else {
altState.lastRewardBlock = blockNumber;
}
Status:
Fixed as per BlockApex recommendation.
Description:
If the governance changes reward type from only Pilot to only Alt and then changes it again from only Alt to Dual, no user can stake their LPs.
Scenario: Before staking starts, if the last reward type is changed twice and is finally set to Dual, it causes the last reward block check, inside the stakeLp() function, to break, i.e., to not equal to start block, causing the getGlobalReward() function to be invoked, which in turn causes the mulDiv() error as the value of totalLpLocked never changed from zero.
function getGlobalReward(
address _vault,
uint256 _blockDiff,
uint256 _multiplier,
uint256 _lastGlobalReward
) private view returns (uint256 _globalReward) {
if (vaultInfo[_vault].totalLpLocked > 0) {
_globalReward = FullMath.mulDiv(
rewardPerBlock,
_multiplier,
1e18
);
_globalReward = FullMath
.mulDiv(
_blockDiff.mul(_globalReward),
1e18,
vaultInfo[_vault].totalLpLocked
)
.add(_lastGlobalReward);
} else {
_globalReward = vaultInfo[_vault].globalReward;
}
}
Remedy:
If the totalLpLocked is zero, the previous state of global reward should return the value of its previous state. In case zero returns, the function should not calculate the new global reward.
function getGlobalReward(
address _vault,
uint256 _blockDiff,
uint256 _multiplier,
uint256 _lastGlobalReward
) private view returns (uint256 _globalReward) {
if (vaultWhitelist[_vault]) {
if (vaultInfo[_vault].totalLpLocked > 0) {
_globalReward = FullMath.mulDiv(
rewardPerBlock,
_multiplier,
1e18
);
_globalReward = FullMath
.mulDiv(
_blockDiff.mul(_globalReward),
1e18,
vaultInfo[_vault].totalLpLocked
)
.add(_lastGlobalReward);
} else {
_globalReward = vaultInfo[_vault].globalReward;
}
} else {
_globalReward = vaultInfo[_vault].globalReward;
}
}
Status:
Fixed as per BlockApex recommendation.
Description:
In an empty farm, if the reward type is changed from only Pilot to only ALT or only Pilot to Dual, the first user who stakes gets a reward of all the empty blocks since the reward type has been updated.
Remedy:
Modify the updateRewardType() function to check if there are no staked LPs in the farm, only then should the start block be updated to set as the current block number.
Status:
Fixed as per BlockApex recommendation.
No issues were found
Description:
if (!vaultWhitelist[_vaultt[i]] && vaultState.totalLpLocked == 0) {
insertVault(_vaultt[i], _multipliert [i]);
} else {
require(!vaultWhitelist[_vault t [i]], "AI");
if (vaultstate reward == RewardType.Dual) {
vaultState. LastRewardBlock = blockNum;
vaultaltState. lastRewardBlock = blockNum;
} else if (vaultstate reward = RewardType.Alt) {
vaultAltState. lastRewardBlock = blockNum;
} else {
vaultState. LastRewardBlock = blockNum;
}
}
Remedy:
initializer() and UpdateRewardType() functions should check in the vault contract if the token actually exists by calling the balanceOf() function.
if (!vaultWhitelist[_vault [i]] && vaultstate.totalLpLocked == 0) {
if (
_rewardType 1 (i) = RewardType. Alt |||
_rewardType 1 [i] == RewardType.Dual
{
require(
IERC20(_rewardToken ^ [i]).balanceof(address(this)) > 0,
"NEB"
);
vaultAltstate, multiplier = _multipliert[i];
vaultAltstate.startBlock = blockNum;
vaultAltState. LastRewardBlock = blockNum;
vaultAltState. rewardToken = _rewardTokent [i];
}
Status:
Fixed as per BlockApex recommendation.
Description:
The event in updateRewardPerBlock() function emits the old reward value as well as updating and sending the new reward value, the problem is that this event emits at the start of the function which manipulates the last block reward for every user in the vault.
function updateRewardPerBlock(uint256 _value)
external
override
onlyGovernance
{
emit RewardPerBlock(rewardPerBlock, rewardPerBlock = _value);
require(_value > 0, "IV");
Remedy:
Event should be fired after the calculation at the end of the function.
function updateRewardPerBlock(uint256 _value)
external
override
onlyGovernance
{
require(_value > 0, "IV");
address[] memory vaults = vaultListed();
for (uint256 i = 0; i < vaults.length; i++) {
if (vaultWhitelist[vaults[i]]) {
if (vaultInfo[vaults[i]].totalLpLocked != 0) {
if (vaultInfo[vaults[i]].reward == RewardType.Dual) {
updateVaultState(vaults[i]);
updateAltState(vaults[i]);
} else if (vaultInfo[vaults[i]].reward == RewardType.Alt) {
updateAltState(vaults[i]);
} else {
updateVaultState(vaults[i]);
}
}
}
}
emit RewardPerBlock(rewardPerBlock, rewardPerBlock = _value);
}
Status:
Fixed as per BlockApex recommendation
Description:
There is a check in updateFarmingLimit() function which does not allow sending zero value in parameter, but it contradicts with the functionality. If the gov sets the farming limit to a specific block and they want to reopen or update the limit they have to send zero value to the updateFarmingLimit() function which will not work if zero value check is placed.
function updateFarmingLimit(uint256 _blockNumber)
external
override
onlyGovernance
{
require(_blockNumber > 0, "IV");
emit UpdateFarmingLimit(
farmingGrowthBlockLimit,
farmingGrowthBlockLimit = _blockNumber
);
updateLastBlock();
}
Remedy:
Zero value check should be removed from the function.
function updateFarmingLimit(uint256 _blockNumber)
external
override
onlyGovernance
{
emit UpdateFarmingLimit(
farmingGrowthBlockLimit,
farmingGrowthBlockLimit = _blockNumber
);
updateLastBlock();
}
Status:
Fixed as per BlockApex recommendation
Description:
No zero address check placed for RewardToken while calling the initializer() function.
function initializer(
address[] calldata _vault,
uint256[] calldata _multiplier,
RewardType[] calldata _rewardType,
address[] calldata _rewardToken
) external override onlyGovernance {
require(_vault.length == _multiplier.length,"LNS");
uint256 blockNum = block.number;
for (uint256 i = 0; i < _vault.length; i++) {
Remedy:
Zero address check should be Placed.
Status:
Fixed as per BlockApex recommendation
Description:
If the updateFarmingLimit() function is called with the same value of the block number in which it is going to be executed (or the past block number), the tx will be mined but it will not limit the farming as expected.
function updateFarmingLimit(uint256 _blockNumber)
external
override
onlyGovernance
{
emit UpdateFarmingLimit(
farmingGrowthBlockLimit,
farmingGrowthBlockLimit = _blockNumber
);
updateLastBlock();
}
Remedy:
A check should be placed in the updateFarmingLimit() function to ensure that block number never equals to current block or past block.
Status:
Fixed as per BlockApex recommendation
Description:
In event of any mishap with $ALT or $PILOT reward, a user won't be able to withdraw their LP funds. A user calls the unstakeLp() function, the contract will throw the “Insufficient balance” error.
Remedy:
Contract should have emergency withdraw() function to withdraw user’s staked LPs
Status:
Fixed as per BlockApex recommendation
Description:
It's extra work for the governance, for the first time if they want to set a vault for only ALT reward they have to call 3 different functions, this work can be done by calling only one function.
function initializer(
address[] calldata _vault,
uint256[] calldata _multiplier
) external override onlyGovernance {
require(_vault.length == _multiplier.length,"LNS");
uint256 blockNum = block.number;
for (uint256 i = 0; i < _vault.length; i++) {
VaultInfo storage vaultState = vaultInfo[_vault[i]];
AltInfo storage vaultAltState = vaultAltInfo[_vault[i]];
Remedy:
RewardType and RewardToken should be set by Calling Initializer() function and later it can be handled by individual functions.
function initializer(
address[] calldata _vault,
uint256[] calldata _multiplier,
RewardType[] calldata _rewardType,
address[] calldata _rewardToken
) external override onlyGovernance {
Status:
Fixed as per BlockApex recommendation
The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report.
This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project.
Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.
Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.
This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.
BlockApex (Auditor) was contracted by Voirstudio (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which took place on _9th November 2021___ .
Name: Unipilot Farming |
Auditor: Moazzam Arif | Muhammad Jarir ud din |
Platform: Ethereum/Solidity |
Type of review: Staking and Farming |
Methods: Architecture Review, Functional Testing, Computer-Aided Verification, Manual Review |
Git repository: https://github.com/VoirStudio/unipilot-farming-contract/tree/dev |
White paper/ Documentation: UnipilotFarm Contract Checklist |
Document log: Initial audit completed on 12th November 2021, Final audit completed on 15th November 2021 |
The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | ERC20 API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Operation Trails & Event Generation |
Unipilot yield farming incentivizes LPs to earn $PILOT by staking their Unipilot NFT of whitelisted pools.
Unipilot yield farming has 1 main smart contract.
UnipilotFarm.sol: Smart contract which allows LPs to earn $PILOT by staking their Unipilot NFTs. UnipilotFarm linearly distributes the $PILOT according to rewardPerBlock and rewardMultiplier
The codebase was audited in an incremental way. Fixes were applied on the way and were re-audited. We used a static analysis tool (slither) which indicated the reentrancy bug in the code. We did manual reviews on the code to find logical bugs and the bugs reported by the automated tools.
The analysis indicates that the contracts audited are working properly.
Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Mythril, MythX and Slither. All the flags raised were manually reviewed and re-tested.
# of issues | Severity of the risk |
1 | Critical Risk issue(s) |
1 | High Risk issue(s) |
2 | Medium Risk issue(s) |
3 | Low Risk issue(s) |
0 | Informatory issue(s) |
Description
The smart contract maintains a global reward per LP share as the pool's globalReward. It is calculated as the following formula:
globalReward = FullMath.mulDiv(blockDifference.mul(temp), 1e18,poolInfo[pool].totalLockedLiquidity).add(_globalReward)
When the last user in the pool withdraws his Unipilot NFT, totalLockedLiquidity is
set to 0. When the next user tries to deposit in the same pool, the contract throws an error (div by 0).
Remedy:
This edge case should be handled.
Dev’s Response:
They acknowledged and fixed by resetting the pool’s variables when the last users removes NFT
Status:
Fixed and Verified
Description:
Unipilot also helps other tokens (specially new tokens) to gain traction by rewarding LPs with this Alt token. So the LPs will be rewarded $PILOT and $ALT_TOKEN. Now when the user claims his reward, Unipilot transfers the shares of $ALT_TOKEN but updates the user reward debt after transferring. Now if the $ALT_TOKEN is ERC777, reentrancy is possible. Although it uses IERC20, but it will work with ERC777 tokens
IERC20(poolAltState.altToken).safeTransfer(userInfo[_tokenId].user, altReward);
poolAltState.lastRewardBlock = block.number;
Remedy:
Update all state variables before transfer. And also use ReentrancyGuard.
Dev’s Response:
We already have used nonReentrant modifier in the new commit
Status
Fixed and verified
Note:
It is good to communicate the changes earlier, so the auditor know beforehand the commit he is auditing
Description:
In depositNft the smart contract assumes that the user shares of liquidity should be less than totalLiquidity in the pool and shares of liquidity should be greater than zero. But in the following line it uses or( || ) instead of and (&&) operator
require(totalLiquidity >= liquidity || liquidity > 0, "IL");
Remedy:
AND (&&) should be used.
Dev’s Response
Acknowledged
Status:
Not Fixed yet
Description:
There is a global variable pilotPerBlock which is used to calculate the $PILOT reward and act as a multiplier. This variable can be updated via governance. But When this variable is updated, it multiplies with the whole duration of staking.
Remedy:
Implement a mechanism like updating the rewardMultiplier of the pool by looping on all pools. Or just remove pilotPerBlock and just use rewardMultiplier to adjust pool rewards
Dev’s Response:
Acknowledged and will loop on all pools
Status:
Fixed and Verified
1. Missing Event in migrateFunds
2. Wrong event in updateUnipilot
3. Remove unused imports like LiquidityAmount.sol
No issues were found
The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report.
This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project.
Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.
Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.
This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.
BlockApex (Auditor) was contracted by KaliCo LLC_ (Client) for the purpose of conducting a Smart Contract Audit/Code Review. This document presents the findings of our analysis which took place from 20th of December 2021 .
Name: LexDAO/KaliDAO |
Auditor: Moazzam Arif | Kaif Ahmed |
Platform: Ethereum/Solidity |
Type of review: Manual code review / Behavioral testing |
Methods: Architecture Review, Functional Testing, Computer-Aided Verification, Manual Review |
Git repository: https://github.com/lexDAO/Kali/tree/299a23a084b8f826f591b30725a3d8b512520ec7 |
White paper/ Documentation: https://github.com/lexDAO/Kali |
Document log Initial Audit: 30th December 2021 (complete) Final Audit: (pending) |
The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | ERC20 API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Complexity of code | Operation Trails & Event Generation |
Kali is a protocol for on-chain organizations inspired by Compound and Moloch DAO Governance. Kali proposals are broken into a variety of types, such that each variance can have their own Governance settings, such as simple/super majority and Quorum requirements.
KaliDAO.sol
KaliDAO is a Comp-style governance into a single contract, it supports extensions to add contracts as apps, for example crowdsale and redemption contracts.
Kali supports hashing and amending docs from deployment and through proposals, providing a hook to wrap organizations into legal templates to rationalize membership rules and liabilities.
KaliDAOtoken.sol
KaliDAOtoken represent voting stakes, and can be launched as transferable or non-transferable, with such settings being updateable via PAUSE proposal. Voting weight can also be delegated, and such weight automatically updates upon token transfers from delegators, incorporating functionality from Comp-style tokens.
Audit log:
In the first two days, we developed a deeper understanding of the DAO and its workings. We started by reviewing the two main contracts against common solidity flaws. After the reconnaissance phase we wrote unit-test cases to ensure that the functions are performing their intended behavior. Then we began with the line-by-line ma
The analysis indicates that the contracts audited are working properly.
After the initial audit, the client was provided with the initial audit report, and the issues reported were discussed. After the fixes had been made, our team performed a re-audit of the codebase. No further issues were found. The contracts were separately reviewed by two individuals. After their thorough and rigorous process of manual testing, an automated review was carried out using Mythril, MythX and Slither. All the flags raised were manually reviewed and re-tested.
# of issues | Severity of the risk |
2 | Critical Risk issue(s) |
3 | High Risk issue(s) |
0 | Medium Risk issue(s) |
0 | Low Risk issue(s) |
4 | Informatory issue(s) |
S# | Findings | Risk | Status |
CR-1. | “Arbitrary call” proposals can lead to unauthorized transfers. | Critical-risk | Acknowledged |
CR-2. | New “Proposals” should not be processed before processing previous “Proposals”. | Critical-risk | Resolved |
HR-1. | A “SUPERMAJORITY” proposal can be triggered without casting any votes. | High-risk | Resolved |
HR-2. | Whitelisting can be bypassed in extension. | High-risk | Acknowledged |
HR-3. | Any malicious user can withdraw “purchaseTokens” and drain the whole contract. | High-risk | Resolved |
IR-1. | Upper and lower bound on Governance params. | Informatory issues | Acknowledged |
IR-2. | “Pause” proposal should escape previous proposals. | Informatory issues | Acknowledged |
IR-3. | Bad error on proposal: “PROCESSED” for non-existent proposals. | Informatory issues | Acknowledged |
IR-4. | Bad error on voting: “VOTING_ENDED” for non-existent proposals. | Informatory issues | Acknowledged |
CR-1. “Arbitrary call” proposals can lead to unauthorized transfers
Contract : KaliDAO.sol
it("proposal type call , using arbitrary calls to call transfer and transferFrom function of kalidao token ", async function () {
// hex data for payload
const tranferFromCall = lexdao.interface.encodeFunctionData("transferFrom", [owner.address,addr3.address, 25]);
// any approved users balance for lexDAO
await lexdao.approve(lexdao.address , 1000000000);
const ownerPreBal = await lexdao.balanceOf(owner.address);
// proposalType.CALL, accounts = [lexdaoAddress], value = 0 ether, call = transferFromCall data
await lexdao.propose(2, "TEST", [lexdao.address] , [0], [tranferFromCall]);
await lexdao.vote(0,true); // votecount check
await forwordTime(40); // vote ended, ignore typo :)
await lexdao.processProposal(0); // process proposal
// after balances
const add3AftBal = await lexdao.balanceOf(addr3.address);
const ownerAftBal = await lexdao.balanceOf(owner.address);
// balances tranferred
expect(add3AftBal.toNumber()).not.eq(0); // tranferFrom call executed
expect(ownerAftBal.toNumber()).lessThanOrEqual(ownerPreBal.toNumber());
});
})
Exploit Scenario
Any malicious user can submit the proposal type call, vote type simple majority and payload has a transferFrom transactions as we created payload on above test. Attackers can transfer any amount from any user if the user has approved his funds to the LexDao contract.
Remedy
1. BlackList specific account addresses and calls.
2. Implement validation of transaction data. (this might create centralization)
Developer's Response
“We believe this issue is avoided by discouraging users from approving token pulls to the `KaliDAO.sol` contract. Instead, payments to DAOs will be through direct transfers or by the use of extension contracts which are authorized to make such token pulls. It is our preference to avoid limiting the kinds of calls that can be made through these kinds of proposals or adding centralization factors”
Auditor's Response
Status : Acknowledged.
CR-2. New “Proposals” should not be processed before processing previous Proposals
Contract : KaliDAO.sol
Function : processProposal()
// skip previous proposal processing requirement in case of escape hatch
if (prop.proposalType != ProposalType.ESCAPE) {
// allow underflow in this case to permit first proposal
unchecked {
require(proposals[proposal - 1].creationTime == 0, 'PREV_NOT_PROCESSED');
}
}
Exploit Scenario
As stated in the piece of code mentioned above, it is very clear that if any previous proposal is pending, no new proposal will be processed. However, this check can be easily by-passed if a non-member adds a proposal and then sponsorProposal is called after that. This way, a new proposal will be processed.
it("Process proposal without processing previous proposals", async function () {
//proposal 0
await lexdao.propose(0, "TEST", [owner.address] , [1], [0x00]);
//proposal 1
await lexdao.propose(0, "TEST", [owner.address] , [1], [0x00]);
//proposal 2 by non-menber
await lexdao.connect(addr3).propose(0, "TEST", [owner.address] , [1], [0x00]);
//sponsor by member
await lexdao.sponsorProposal(2);
await forwordTime(40);
try {
//process Proposal without process previous proposals
await lexdao.processProposal(3);
} catch (error) {
console.log(error);
}
});
Remedy
Do not create a “new” proposal when sponsorProposal() is called.
Developer's Response
“We have made a fix by tracking a new global state variable, currentSponsoredProposal and updating it upon every sponsored proposal. This value gets appended to the Proposal struct and is checked on processing, like so:
if (proposals[prop.prevProposal].creationTime != 0) revert PrevNotProcessed();
Auditor's Response
Status : Resolved.
HR-1. A “SUPERMAJORITY” proposal can be triggered without casting any votes
Contract : KaliDAO.sol
Function : _countVotes()
function _countVotes(
VoteType voteType,
uint256 yesVotes,
uint256 noVotes
) internal view virtual returns (bool didProposalPass) {
// rule out any failed quorums
if (voteType == VoteType.SIMPLE_MAJORITY_QUORUM_REQUIRED || voteType == VoteType.SUPERMAJORITY_QUORUM_REQUIRED) {
uint256 minVotes = (totalSupply * quorum) / 100;
// this is safe from overflow because `yesVotes` and `noVotes` are capped by `totalSupply`
// which is checked for overflow in `KaliDAOtoken` contract
unchecked {
uint256 votes = yesVotes + noVotes;
if (votes < minVotes) return false;
}
}
// simple majority
if (voteType == VoteType.SIMPLE_MAJORITY || voteType == VoteType.SIMPLE_MAJORITY_QUORUM_REQUIRED) {
if (yesVotes > noVotes) return true;
// super majority
} else {
// example: 7 yes, 2 no, supermajority = 66
// ((7+2) * 66) / 100 = 5.94; 7 yes will pass
uint256 minYes = ((yesVotes + noVotes) * supermajority) / 100;
if (yesVotes >= minYes) return true;
}
}
Exploit Scenario
Let's consider the code above; The else() statement : if yesVotes and noVotes are zero and supermajority is set to 66, the resulting minYes will ultimately be zero as well. Now let’s see the last if() inside the else( ) condition. Both yesVotes and minYes are zero, so the condition will return true. If any malicious user adds a proposal of mint/burn, they just have to wait to process the proposal because that proposal does not need any votes.
Remedy
Remove the equality sign and ensure that the voting period is long enough so that the users are able to vote on every proposal. This will make sure that the yesVotes required to process a proposal is greater than 0 and can get votes from other users as well.
Developer's Response
We have a made a fix to this issue by putting in a return check at the top of the “_countVotes” internal function that fails a proposal if nobody participated, like so: “if (yesVotes == 0 && noVotes == 0)”
return false;
Auditor's Response
Status : Resolved.
HR-2. Whitelisting can be bypassed in extension
Contract : KaliDAOcrowdsale.sol
if (sale.listId != 0) require(whitelistManager.whitelistedAccounts(sale.listId, account),
'NOT_WHITELISTED');
Exploit Scenario
User can bypass the whitelisting check by providing listId “0”.
(Note: This might be a false assumption because we didn't properly recon the latest codebase at this point)
Remedy
A contract should have proper checks that restrict users to set extension with “0” listId.
Developer's Response
In the crowdsale extension contract, we purposefully allow users to set a null value for whitelist ID in order to allow for unrestricted token sales.
Auditor's Response
Status : Acknowledged.
HR-3. Any malicious user can withdraw “purchaseToken” and drain the whole contract
Contract : KaliDAOcrowdsale.sol
it("Bypassing whitelisting can lead to unautherized transfers " , async function() {
// Data for crowdsale Extension
const data = abiCoder.encode(
["uint256" , "address" , "uint8" , "uint96" , "uint32"] ,
[0 , token.address , 1, BigNumber("100000").toString() , BigNumber("1640838044").toString()]);
//transfering funds to address 3
await token.transfer(addr3.address , BigNumber("100").toString());
//checking balance before
console.log(await token.balanceOf(addr3.address));
console.log(await token.balanceOf(addr4.address));
//approving funds by address 3 to crowdsale contract
await token.connect(addr3).approve(crowdsale.address , BigNumber("100").toString());
//set extension using address 4
await crowdsale.connect(addr4).setExtension(data);
//call extension using address 4
await crowdsale.connect(addr4).callExtension(addr3.address, BigNumber("100").toString());
//checking balance after
console.log(await token.balanceOf(addr3.address));
console.log(await token.balanceOf(addr4.address));
})
Exploit Scenario
Consider that a user can set an extension using the listId “0” (the check which can be easily bypassed). Now if any user has approved their funds to the crowdsale contract, any malicious user can simply call the setExtension() using the token address in his data and then call callExtension() using address 3 (used in the example above) in the parameters which will ultimately withdraw all the funds user have approved to the crowdsale contract.
Remedy
Whitelisting should be handled properly like funds should be transferred to a whitelisted address instead of msg.sender.
Developer's Response
Resolved the issue by limiting users to call the extension directly from crowdsale and deducting amounts form msg.sender instead of any address.
Auditor's Response
Status : Resolved.
IR-1. Upper and lower bound on Governance params
Contract : KaliDAO.sol
Description
init function has proper upper and lower bounds but if the user set values by Governance, it has no lower and upper bound check. Unchecked math assumes these bounds.
Remedy
A proper upper and lower bounds check should be placed while changing values by Governance.
Developer's Response
We provide reversion checks on governance params in the “propose()” function to ensure that proposals to amend “votingPeriod”, “quorum”, “supermajority” and proposal types are kept within expected bounds.
Auditor's Response
Status : Acknowledged.
IR-2. “Pause” proposal should escape previous proposals
Contract : KaliDAO.sol
Description
To pause the contract, a proposal is submitted. Most of the time a pause is needed in emergencies. To timely execute a pause proposal it should not wait for previous proposals. (We have seen mishaps with $COMP in the past, where their proposal to fallback takes days).
Remedy
There should be a check in processProposal() that if the proposal type is “pause” it can execute right away.
Developer's Response
It is our preference to maintain a voting period for PAUSE proposal types in order to accommodate the use case of gradual decentralization by some DAOs. For example, there are non-emergency situations where a DAO might deploy with a closed founder group, grow its membership through proposals, but then want to vote and reach consensus on making membership tokens transferable, and therefore, open to the public.
Auditor's Response
Status : Acknowledged.
IR-3. Bad error on proposal: “PROCESSED” for non-existent proposals Contract : KaliDAO.sol
function processProposal(uint256 proposal) public nonReentrant virtual returns (
bool didProposalPass, bytes[] memory results
) {
Proposal storage prop = proposals[proposal];
require(prop.creationTime != 0, 'PROCESSED');
Description
If a user tries to process a non-existent proposal, the above code won’t let him process because proposal creation time is 0 but the user will get the “PROCESSED” error. This should be handled properly by separately checking if the proposal actually exists or not.
Remedy
“Proposal does not exist” should be displayed by adding a check using require. Developer's Response
We have provided a clarified reversion message, “Processed()” -> “NotCurrentProposal()”. We otherwise would like to avoid additional checks to optimize for gas efficiency.
Auditor's Response
Status : Acknowledged.
IR-4. Bad error on voting: “VOTING_ENDED” for non-existent proposals Contract :
// this is safe from overflow because `votingPeriod` is capped so it will not combine
// with unix time to exceed 'type(uint256).max'
unchecked {
require(block.timestamp <= prop.creationTime + votingPeriod, 'VOTING_ENDED');
}
KaliDAO.sol
Description
If a user tries to vote on a non-existent proposal, the above code won't let him vote because proposal creationTime + votingPeriod will be less than block.timestamp but the user will get “VOTING_ENDED” error. This should be handled properly by separately checking if the proposal actually exists or not.
Remedy
“Proposal does not exist” should be displayed by adding a check using require.
Developer's Response
We have provided a clarified reversion message, “VotingEnded()” -> “NotVoteable()”. We otherwise would like to avoid additional checks to optimize for gas efficiency.
Auditor's Response
Status : Acknowledged.
The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report.
This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project.
Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.
Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.
This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.
Name: Dafi bridge (Final Audit) |
Auditors: Moazzam Arif | Muhammad Jarir |
Platform: Ethereum/Solidity |
Type of review: ETH - BSC Bridge |
Methods: Architecture Review, Functional Testing, Computer-Aided Verification, Manual Review |
Git repository: https://github.com/DAFIProtocol/dBridge/tree/dafi-bridge-contracts |
White paper/ Documentation: Dafi Bridge Flow Document (1).pdf |
Document log: Initial Audit: 30th November 2021 Final Audit: 2nd December 2021 |
The git-repository shared was checked for common code violations along with vulnerability-specific probing to detect major issues/vulnerabilities. Some specific checks are as follows:
Code review | Functional review | |
Reentrancy | Unchecked external call | Business Logics Review |
Ownership Takeover | ERC20 API violation | Functionality Checks |
Timestamp Dependence | Unchecked math | Access Control & Authorization |
Gas Limit and Loops | Unsafe type inference | Escrow manipulation |
DoS with (Unexpected) Throw | Implicit visibility level | Token Supply manipulation |
DoS with Block Gas Limit | Deployment Consistency | Asset’s integrity |
Transaction-Ordering Dependence | Repository Consistency | User Balances manipulation |
Style guide violation | Data Consistency | Kill-Switch Mechanism |
Costly Loop | Operation Trails & Event Generation |
A detailed overview of the project can be found here:
https://blog.dafiprotocol.io/introducing-the-dbridge-testnet-c564f5b4eea2
Dafi’s “dbridge” enables users to bring their ERC-20 $DAFI tokens across from the Ethereum network to Binance Smart Chain, and vice versa, with aims of making $DAFI available on multiple high-speed and low-cost networks. As Dafi’s goal is to support every token on most chains, to launch their own dToken, it’s important that their protocol is cross-chain.
Dafi Bridge is an implementation of a generic POA Bridge. Authority is distributed among validators. Validators sign the proof-of-burn message and the user submits (to avoid gas griefing attacks) the signature on the alternate chain to claim tokens.
The codebase:
The system consists of 3 smart contracts (i.e ETH Bridge, BSC Bridge & a burnable/mintable ERC20 token representing Dafi on alternate chains)
Bridge contracts have onlyOwner modifier to set configurations (i.e adding/removing validators, minimum signers required(threshold)).
The analysis indicates that the contracts audited are working properly.
Our team performed a technique called “Filtered Audit”, where the contract was separately audited by two individuals. After their thorough and rigorous process of manual testing, no potential flags were raised.
# of issues | Severity of the risk |
0 | Critical Risk issue(s) |
0 | High Risk issue(s) |
1 | Medium Risk issue(s) |
1 | Low Risk issue(s) |
3 | Informatory issue(s) |
No critical-risk issues were found in the review.
No high-risk issues were found in the review.
1. If ETHToken is changed, tokens will be locked forever in the contract
File: ETHBridgeOptimized.sol
Description:
Owner(multisig) can change the underlying ETHToken address. If there are tokens locked in the smart contract and changeToken() is called, tokens will be locked forever in the contract.
Remedy:
Remove changeToken() method from the contract. If there is a need to change the token address, a new contract can be deployed.
Status:
Fixed.
1. Unnecessary allowance check in burn/lock tokens
File: ETHBridgeOptimized.sol, BinanceBridgeOptimized.sol
Description:
if (IERC20(BSCTOKEN).allowance(msg.sender, address(this)) < amount)
revert AllowanceInsufficient(
IERC20(BSCTOKEN).allowance(msg.sender, address(this)), amount);
When burning/locking tokens, the contract checks if the msg.sender has allowed the smart contract (address(this)) to burn/lock. This check will cost extra gas. Normally these checks are used with burnFrom. There is already check placed in the burn method of dafiToken.sol
Remedy:
Remove allowance checks
Status:
Fixed.
1. Misleading variable/function names
File: ETHBridgeOptimized.sol, BinanceBridgeOptimized.sol, dafiTokenBSC.sol
function burnTokens(uint256 amount, address targetChain) external {
function lockTokens(uint256 amount, address targetChain) external {
function viewOwners(address checkAddress) external view returns (bool) //viewValidators
function burn(uint256 _value, address _beneficiary) external onlyBridge { // burnFrom
Remedy:
Our suggestion is that
1. targetChain should be recipient
2. viewOwners should be ViewValidators
2. burn should be burnFrom in dafiToken
2. Gas cost optimization while incrementing nonce
File: ETHBridgeOptimized.sol, BinanceBridgeOptimized.sol
Description:
nonceIncrement() method is used to increment a state variable nonce. Calling a function instead of directly updating the state variable will save the gas cost. Calling a function introduce JUMP opcode which has a higher gas cost
Status:
Fixed.
3. Centralization risk on minting/burning on BSC Token
Description:
onlyBridge can burn/mint tokens. Bridge address can be changed by the owner (MultiSig). There are no potential risks as long as the signers of the multisig are honest.
The smart contracts provided by the client for audit purposes have been thoroughly analyzed in compliance with the global best practices till date w.r.t cybersecurity vulnerabilities and issues in smart contract code, the details of which are enclosed in this report.
This report is not an endorsement or indictment of the project or team, and they do not in any way guarantee the security of the particular object in context. This report is not considered, and should not be interpreted as an influence, on the potential economics of the token, its sale or any other aspect of the project.
Crypto assets/tokens are results of the emerging blockchain technology in the domain of decentralized finance and they carry with them high levels of technical risk and uncertainty. No report provides any warranty or representation to any third-Party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third-party should rely on the reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project.
Smart contracts are deployed and executed on a blockchain. The platform, its programming language, and other software related to the smart contract can have its vulnerabilities that can lead to hacks. The scope of our review is limited to a review of the Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks.
This audit cannot be considered as a sufficient assessment regarding the utility and safety of the code, bug-free status or any other statements of the contract. While we have done our best in conducting the analysis and producing this report, it is important to note that you should not rely on this report only - we recommend proceeding with several independent audits and a public bug bounty program to ensure security of smart contracts.