Smart Contract Audit
Introductionβ
CoinFabrik was asked to audit the contracts for the BankX project. First we will provide a summary of our discoveries, and then we will show the details of our findings.
Scopeβ
The audited files are from the git repository located at
www.github.com/Lance-Parker/BankX git repository, commit
e99ff5f8cfe5697f2ddae7b1eb90cc3176659cf4, and in the
www.github.com/Lance-Parker/CD git repository, commit
14a850049458348e6d4b1e8b2ce1e97ba678ca02.
The audited files are:
- BankX/contracts/BankX/BankXToken.sol: Staking token implementation
- BankX/contracts/BankX/BankX_premint.sol: Wrapper to BankXToken
- BankX/contracts/XSD/Pools/XSDWETHpool.sol: XSD/WETH pool
- BankX/contracts/XSD/Pools/BankXWETHpool.sol: BankX/WETH pool
- BankX/contracts/ERC20/ERC20Custom.sol: Modified ERC20 contract
- BankX/contracts/XSD/Pools/CollateralPool.sol: Collateral pool
- BankX/contracts/XSD/Pools/CollateralPoolLibrary.sol: Pool utility functions
- CD/contracts/interfaces/BankXInterface.sol: Interface to BankX Token
- CD/contracts/CD.sol: Certificate of Deposit main contract
- CD/contracts/GlobalsAndUtility.sol: Utility functions and global variables
- CD/contracts/StakeableToken.sol: Staking logic
The scope of the audit is limited to those files. No other files in this repository were audited. Its dependencies are assumed to work according to their documentation. Also, no tests were reviewed for this audit.
Analysesβ
Without being limited to them, the audit process included the following analyses:
- Arithmetic errors
- Outdated version of Solidity compiler
- Race conditions
- Reentrancy attacks
- Misuse of block timestamps
- Denial of service attacks
- Excessive gas usage
- Missing or misused function qualifiers
- Needlessly complex code and contract interactions
- Poor or nonexistent error handling
- Insufficient validation of the input parameters
- Incorrect handling of cryptographic signatures
- Centralization and upgradeability
Summary of Findingsβ
We found a critical issue, a medium issue and a minor issue. Also, several enhancements were proposed.
Security Issuesβ
ID | Title | Severity | Status |
---|---|---|---|
CR-01 | Manipulation of Daily Payout | Critical | Acknowledged |
ME-01 | Solidity Compiler Version | Medium | Resolved |
MI-01 | Hardcoded Interest Rate And Launch Time | Minor | Resolved |
Summary of Findingsβ
Severity Classificationβ
Security risks are classified as follows:
Critical: These are issues that we manage to exploit. They compromise the system seriously. They must be fixed immediately .Medium: These are potentially exploitable issues. Even though we did not manage to exploit them or their impact is not clear, they might represent a security risk in the near future. We suggest fixing them as soon as possible.
Minor: These issues represent problems that are relatively small or difficult to take advantage of, but can be exploited in combination with other issues. These kinds of issues do not block deployments in production environments. They should be taken into account and be fixed when possible.
Issues Statusβ
An issue detected by this audit can have four distinct statuses:
- Unresolved The issue has not been resolved.
- Acknowledged The issue remains in the code, but is a result of an intentional decision.
- Resolved Adjusted program implementation to eliminate the risk.
- Partially resolved: Adjusted program implementation to eliminate part of the risk. The other part remains in the code, but is a result of an intentional decision.
- Mitigated Implemented actions to minimize the impact or likelihood of the risk
Critical Severity Issuesβ
CR-01 Manipulation of Daily Payout
Locationβ
- contracts/GlobalsAndUtility.sol:495 (CD)
The dailyDataUpdate() calculates the daily payout taking in account the BankX token total supply amount:
rs._allocSupplyCached = bankXContract.totalSupply() + g._lockedXsTotal;
And then at _dailyRoundCalc(), line 471
rs._payoutTotal = rs._allocSupplyCached * 10000 / 171779836;
And then at _dailyRoundCalcAndStore(), line 483:
dailyData[day].dayPayoutTotal = uint72(rs._payoutTotal);
But the bankX supply can be manipulated by any authorized pool, In this way the daily payout can be manipulated, by increasing or decreasing the supply of BankX tokens and calling dailyDataUpdate() at appropriate times.
Recommendationβ
Restrict the dailyDataUpdate() function so the payout cannot be manipulated by an external entity
Statusβ
Acknowledged. According to the development team, this issue will be mitigated because after launch, admin keys will be renounced.Medium Severity Issuesβ
ME-01 Solidity Compiler Versionβ
All the audited files use the statement:
pragma solidity 0.5.13;
This causes that very old solidity compiler version being used which may lead into hitting already fixed bugs. Normally this issue is of minor severity but due to the age of the solidity compiler specified (0.5.13) and the amount of bug fixes accumulated since that version, it was upgraded to Medium severity.
Recommendationβ
It is better to lock to a specific compiler version (for example, pragma solidity 0.8.13;) and keep it up to date, fully testing on each upgrade.
Statusβ
ResolvedMinor Severity Issuesβ
MI-01 Hardcoded Interest Rate And Launch Time
Locationβ
- contracts/GlobalsAndUtility.sol:103,471 (CD)
The Launch date is hardcoded:
uint256 internal constant LAUNCH_TIME = 1575331200;
Also, the interest rate used to calculate the daily payout is a hard-coded constant:
rs._payoutTotal = rs._allocSupplyCached * 10000 / 171779836
Recommendationβ
The LAUNCH_TIME constant could be a variable initialized at contract initialization. Also, a mechanism to update the interest rate should be implemented.
Statusβ
ResolvedEnhancementsβ
These items do not represent a security risk. They are best practices that we suggest implementing.
Tableβ
ID | Title | Status |
---|---|---|
EN-01 | SafeMath Not Needed in Solidity >=0.8 | Implemented |
EN-02 | Lack of Zero-Address Verification | Implemented |
EN-03 | Different Reentrancy-Guard Methods | Implemented |
Detailsβ
EN-01 SafeMath not needed in Solidity >=0.8
Locationβ
- contracts/BankXToken.sol:2
- contracts/BankX_premint.sol:2
Solidity version 0.8 and over automatically reverts on integer overflows, not needing the SafeMath.sol contract anymore.
Recommendationβ
Remove the SafeMath.sol contract from the imports section and refactor the code appropriately.
Statusβ
ImplementedEN-02 Lack of zero-address Verification
Locationβ
- contracts/BankXWETHpool.sol:223
A zero-check on the new address should be added to the function
setSmartContractOwner().Statusβ
ImplementedEN-03 Different Reentrancy-Guard Methods
Locationβ
- contracts/CollateralPool.sol:69
CollateralPool.sol uses a custom lock, while XSDWETHpoo.sol and BankXWETHpool.sol uses OpenZepellinsβs reentrancy guards, which is a better approach.
Recommendationβ
Replace custom guard code for OpenZepellinsβs reentrancy guards
Statusβ
ImplementedOther Considerationsβ
Arbitrary Bonus Creationβ
Location- contracts/NFTBonus.sol:22 (CD)
Any user can get an arbitrarily big bonus by setting any number of OwnerIDs using NFTBonus.sol:setOwnerIds(). The function stackStartBonusXs() then will return an arbitrarily big bonus using this operation:
return bonusXs + bonusXs * nftBonusContract.getNftsCount(msg.sender) / 10;
Here, the getNftsCount(msg.sender) can return a multiplier controlled by the attacker, so bonusXs can be arbitrarily big. This is a critical bug as it allows stealing
tokens from the contract but as this contract was out-of-scope for this audit, it was not added to the list of security issues, however itβs documented in this section.
Recommendationβ
Restrict the NFTBonus contract so only the owner can add or remove NFT ids.
Statusβ
BankX responded that this was a test contract and because of that, it has no security impact.
Centralizationβ
The contracts Pools/XSDWETHpool.sol, Pools/BankXWETHpool.sol and others use the Chainlink service as the price oracle.
Using an average or a backup Oracle solution might mitigate centralization, but are more costly, and security of a multi-oracle solution is not guaranteed.
Oracles are complex and approaches to decentralize come with security, accuracy and reliability tradeoffs. Chainlink oracles are powered by independent, professional and decentralized node operators that use multiple layers of aggregation - https://blog.chain.link/chainlink-price-feeds-secure-defi/
Statusβ
This is not a security issue and it doesn't require a modification. It's just an observation about using a single Oracle vs using an average or a backup Oracle, solutions that improves security and mitigates centralization, but are more complex and costly. For more information about Oracle manipulation and implementation best-practices visit this article: www.consensys.github.io/smart-contract-best-practices/attacks/oracle-manipulation/
Upgradesβ
No upgrade mechanism exists in any contract audited.
Privileged Rolesβ
These are the privileged roles that we identified on each of the audited contracts.
BankX/contracts/BankX/BankXToken.sol
Ownerβ
The Owner can: Set the pool, set the Timelock address, set the Treasure address, set the XSD token address.
Timelockβ
The Timelock address can: Set the pool, set the Timelock address, set the Treasure address, set the XSD token address
Poolsβ
The XSD pools can: Mint new BankX tokens, burn BankX tokens. BankX/contracts/BankX/BankX_premint.sol
Ownerβ
The Owner can: Set the BankX token address, set the XSD token address. BankX/contracts/XSD/Pools/BankXWETHpool.sol
Ownerβ
The Owner can: Initialize the contract, change the owner address. BankX/contracts/XSD/Pools/CollateralPool.sol
Ownerβ
The Owner can: Initialize the contract, change the owner address
Treasuryβ
The Treasury address can: Add new liquidity Tokens, add liquidity ETHs. CD/contracts/GlobalsAndUtility.sol
Ownerβ
The Owner can: Set the BankX contract address, set the NFTBonus contract address
Changelogβ
2022-08-05 β Initial report based on commit
e99ff5f8cfe5697f2ddae7b1eb90cc3176659cf4 for repository
https://github.com/Lance-Parker/BankX and
14a850049458348e6d4b1e8b2ce1e97ba678ca02 for the repository
https://github.com/Lance-Parker/CD. .2022-08-23 β Fixes checked on commit
f127336bd6a8390c52fa6b2a93bde1306f529745 for repository
[https://github.com/Lance-Parker/BankX](https://github.com/Lance-Parker/BankX and
E096bbce99cafb6e363b16dffec1ce3604c837d6 for repository
https://github.com/Lance-Parker/CD. .