AI audit

[ibjc]

Courtesy of @yubrew, we received an AI contract audit!

Note that it is an alpha product/service, so keep in mind of potential ai "hallucinations".

here is the full audit report:

1. Reentrancy: The InverseBondingCurve contract transfers Ether in several functions includingremoveLiquidityandsellTokens`. However, it does not follow the Checks-Effects-Interactions pattern properly, meaning that state changes happen after the external calls. This may potentially allow for reentrancy attacks. Although such an attack isn't straightforward, because it would require the recipient to be a contract, it is a potential issue.

2. The code lacks input validation for several externally-facing functions. For instance, transfer, transferFrom, and the ERC20 contract's approve method lack input validation. An attacker may be able to gain unauthorized control of funds without the user's knowledge.

3. There is no permission check on the method mint. The only thing preventing anyone from creating new tokens is a simple onlyOwner modifier. This can potentially lead to a massive supply of tokens if an attacker can take control of the owner's account.

4. Lack of event logging: Key actions such as token transfers and state changes do not emit events, which makes tracking and auditing past transactions and changes difficult.

5. The _calculateAndUpdateFee function is underspecified, and combined with the lack of input validation, might be a source of vulnerabilities. Specifically, the return value of tokenAmount.mulDown(_lpFeePercent) is subtracted from tokenAmount, but since _lpFeePercent is user-controlled, an attacker can cause integer underflow here.

Suggest to use 'SafeMath' for all arithmetic operations to avoid integer underflow/overflow.

6. Function visibility: Several functions are marked as private, which means they can only be called from within the contract itself. This lack of visibility increases the difficulty of contract interaction and testing, and could potentially hide bugs.

To exploit some of these vulnerabilities, specially for those where user input is not validated, it suffices to drive the contract into an undesired state (for example, by manipulating _lpFeePercent) and then call _calculateAndUpdateFee. For the ownership vulnerability, if the owner's account is compromised tokens can be minted at will. To exploit a potential reentrancy attack, one could for example write a contract that calls removeLiquidity, and in its fallback function call removeLiquidity again.

1) No vulnerability: All the contracts using “.call{value: _}” are properly using it. No reentrancy attack, DoS attack, or Ether theft could be performed.

2) Precision vulnerabilities: While no exploitable vulnerability was found, it is important to note that the methods "initialize", "addLiquidity", "removeLiquidity", "buyTokens" and "sellTokens" do present a precision problem due to the use of the "divDown" method from FixedPoint library, which may result in rounding errors, causing small imprecisions in calculations. This issue could be potentially exploited by calling these methods in a manner that maximizes rounding errors, facilitating imbalance and provide unexpected advantages in some specific and edge scenarios. Here is an example of how such an attack could look like:

function attackPrecision(address target) public payable {
    InverseBondingCurve curve = InverseBondingCurve(target);
    uint256 oldPrice = curve.priceOf(curve.totalSupply());
    for(uint256 i=0; i < 10000; i++) {
        curve.buyTokens{value: MIN_LIQUIDITY}(address(this), 1);
        uint256 newPrice = curve.priceOf(curve.totalSupply());
        if (oldPrice != newPrice) { // Due to rounding errors price changes
            curve.sellTokens(address(this), MIN_SUPPLY, 1); 
            // Sell at new price and get more ether than spent due to rounding error
        }
        oldPrice = newPrice;
    }
}

This kind of precision attacks are complex and rare, and decided to be accepted by developers in many DeFi projects due to the high cost of performing them compared to potential gains. However, it is generally good to be aware of such issues and ensure there are maximum limits for any user actions to minimize potential impact.

3) Lack of input sanitization: All the functions of the contract assume that the owner and the users will use the contract properly. These assumptions can act as a potential vulnerability, always ensure to validate inputs where necessary.

Here are a few possible vulnerabilities I found in the contract:

1. DoS with Unpredicted Gas Limit: In the functions sellTokens() and removeLiquidity(), the contract attempts to transfer ETH using a low-level .call(). If the receiving contract's fallback function runs out of gas or if the transfer fails for any other reason, these functions will revert which opens your contract up to a Denial of Service (DoS) attack.

For sellTokens():

(bool sent,) = recipient.call{value: returnLiquidity}("");
require(sent, "Failed to send Ether");

For removeLiquidity():

(bool sent,) = recipient.call{value: returnLiquidity}("");
require(sent, "Failed to send Ether");

2. Reentrancy: The contract is potentially vulnerable to a reentrancy attack as it calls user-supplied addresses, both in the sellTokens() and removeLiquidity() functions, before state updates. Although currently the state is updated before the funds are sent, if state updates are added after these calls in the future, the contract risk a reentrancy attack.

For sellTokens():

_inverseToken.burnFrom(msg.sender, burnToken);
IERC20(_inverseToken).safeTransferFrom(msg.sender, address(this), fee);
(bool sent,) = recipient.call{value: returnLiquidity}("");

For removeLiquidity():

_burn(msg.sender, amount);
(bool sent,) = recipient.call{value: returnLiquidity}("");

Recommendations:

• Always make sure that all internal state changes happen before calling external contracts.
• Consider using OpenZeppelin's ReentrancyGuard for functions that are vulnerable to reentrancy attacks.
• Instead of low-level .call()s, use Solidity's built-in mechanisms for sending Ether such as transfer().

After observing the given smart contract codes, there are no explicit security vulnerabilities found. However, there are some potential dangers or points that the developer should pay attention to:

1. Integer Underflow/Overflow: In functions like buyTokens and sellTokens, arithmetic operations are taking place, and although Solidity 0.8.x has built-in arithmetic overflow and underflow protection, for the older version, please ensure to use the SafeMath library to prevent potential arithmetic underflow or overflow errors.

2. Upgradable Smart Contract: As an upgradable smart contract, it's crucial to protect proxy-based contracts to avoid the risk of being pwned if you get ownership of the proxy. This contract should test updates end to end and with manual analysis.

3. Initializable contracts: Functions marked as initializer can only be called once right after contract deployment. It's important to ensure the function does all the setup it needs to prevent future state corruption.

4. Gas Limit: Some loops in this contract could exceed the gas limit if the array it's looping through grows too large.

5. Use external over public: In order to optimize gas, it's better to use external keyword over public for functions that will not be called internally.

6. Re-entrancy attack: Even though no explicit reentrancy attack vulnerability is found (as there is no call to an untrusted contract followed by state-changing operations), please always consider using ReentrancyGuard from the OpenZeppelin library for state-changing functions to protect your contract from reentrancy attacks.

7. Error Message Information Leakage: Writing too much info in error messages could potentially yield information risks.

8. Ensure proper access control: Keep careful control over which addresses have the ability to call specific functions. In the code provided, these seem to be appropriately restricted, but should be a point of consideration especially with the proxy upgrade pattern.

Remember, it's always important to conduct a complete official audit using automatic tools and manual checking by a skilled security expert before the contract goes live.

1. Re-Entrancy:

   • The removeLiquidity, buyTokens, and sellTokens methods are susceptible to re-entrancy attack as they transfer the funds before updating the status. An attacker can reenter the smart contract and drain its funds.
   • Recommended fix: Use a mutex or implement the Checks-Effects-Interactions patter to prevent re-entrancy.

2. Front Running:

   • The buyTokens and sellTokens methods are vulnerable to front-running. Their prices are public, so miners or users with faster confirmation times can take advantage of better prices and traders can be victim to MEV (Miner Extractable Value).
   • Recommended fix: To prevent front-running, consider use of a commit-reveal scheme or batched transactions.

3. No Limit On gas:

   • In claimProtocolReward method there is no limit on the gas which can potentially lock a smart contract.
   • Recommended fix: Put a limit on gas for transactions.

4. Unprotected selfdestruct Function:

   • The contract doesn’t have a selfdestruct function, but if it has, it can be a serious issue to destroy a contract without any control or limit.
   • Recommended fix: Make sure to control the access to selfdestruct function.

5. Missing event logs:

   • There are not events emitted after important steps like _authorizeUpgrade in InverseBondingCurve. An off-chain service won't be able to be notified of such event.
   • Recommended fix: Emit an event after this action for external services to hook on.

6. Ownership controls:

   • It's also important to ensure that sensitive functions, such as those that can update contract parameters, are adequately protected by ownership controls.

Methods can be exploited: buyTokens, sellTokens, addLiquidity, removeLiquidity, initialize, claimProtocolReward, updateFeeOwner.

The contract code does not have any critical vulnerabilities. It's well-written with safe math operations, error checks, and requires suitable permissions for critical methods. However, there are a few areas to improve:

1. Consider Using IERC20.transfer rather than call:

   In the removeLiquidity function, the contract is using a low-level call to transfer Ether. This might expose the risk of reentrancy attacks. You can use the safe IERC20.transfer instead.

   (bool sent,) = recipient.call{value: returnLiquidity}("");
   require(sent, "Failed to send Ether");

   Recommended update:

   IERC20(WETH).transfer(recipient, returnLiquidity);

2. External Calls After State Modifications:

   It's recommended that external calls should be the last actions in a function due to the reentrancy attack. There are several places in the methods removeLiquidity, sellTokens, and unstake where some state changes (_burn, _totalStaked decrease) are made after Ether transfer.

   For instance, in removeLiquidity:

   (bool sent,) = recipient.call{value: returnLiquidity}("");
   ...
   _burn(msg.sender, amount); 

   Consider doing state modifications before Ether transfer.

3. Defensive Payment Programming:

   It would be advisable to employ a "Check-Effects-Interaction" pattern especially in the removeLiquidity and sellTokens function where user balance is updated.

   _burn(msg.sender, amount); 
   (bool sent,) = recipient.call{value: returnLiquidity}("");

4. No Proper Handling of Contract Interactions:

   The InverseBondingCurve contract interacts with other contracts such as IInverseBondingCurveToken. This exposes the contract to risks if the other contract is compromised or does not act as expected.

5. The comment 'TODO: add logic for transfer owner' could be a vulnerability:

   If there's no logic for transferring ownership on the InverseBondingCurveToken contract, then this function should be created to evade a situation where there doesn't exist the possibility to transfer ownership if needed.

6. Re-entrancy Attack - As the withdrawLiquidity function has a state change that happens after the external call, this smart contract can be vulnerable to a re-entrancy attack. It's generally a best practice to first change the state and then transfer ETH token.

7. Arithmetic Over/Under-Flows - Use SafeMath when handling arithmetic operations. Overflows and underflows can lead to unexpected results.

8. In the claimReward function, the else statement is empty which does nothing for any other rewardType not covered in the if-else statements. It's good practice to revert in the case where an invalid rewardType is passed.

9. Fee Escalation - An owner can manipulate the system to their advantage by manipulating fees through updateFeeConfig.

10. Oracle Manipulation - The price of tokens is calculated within the contract using user-provided values. This introduces the risk of price manipulation. Use oracles if available for these types of calculations.

11. In the updateFeeOwner function, there is no check to prevent setting the fee owner to the zero address, this would effectively burn the fees.

12. Lack of event logging -: Events are not being emitted after state changes in mint and burnFrom functions of the InverseBondingCurveToken contract making it difficult to track state changes.

13. transfer and transferFrom functions do not revert if _updateLpReward encounters an error. Instead, they continue execution. This can be detrimental to the functionality of the contract.

14. In the claimProtocolReward function, there is no check that amount is greater than zero before the transfer to the protocolFeeOwner is done.

15. In unstake function, the check for the user's staking balance should come before _updateStakingReward(msg.sender) is called.

16. The contract uses call{value: returnLiquidity}("") to send Ether, which can make the contract vulnerable to fall back or by triggering unexpected behaviours in receiving contracts.

17. claimReward method does not ensure that the reward belongs to the user claiming or that any reward exists.

18. Vulnerable to Front-Running Attacks: Transaction ordering and timing can be manipulated by miners or users who are willing to pay a high gas price. These users can potentially "front-run" transactions and adversely affect other user's transactions.

19. The transfer and transferFrom functions in the ERC20Upgradeable contract can be paused by the contract owner, however, the methods in the contract do not consider the paused status.

20. Missed Function Access Control: Functions initialize, updateFeeConfig and updateFeeOwner should ideally be restricted to be called only by the contract owner, but as implementation is not included, these can be triggered by any address.

21. The contract tends to overuse of low-level calls call, delegatecall. It's usually better to use high-level calls unless you know what you're doing.

22. Gas Optimization: There is room for gas optimization in the code, as multiple repetitive calls to the same function or identical calculations are made.

23. Always use proper naming for methods, variables and event. It might not be security-related but in long term, these contracts are easy to understand & less prone to bug.

Please make the necessary corrections to mitigate these vulnerabilities.`

[ibjc]

the audit report does seem like a combination of multiple model runs

[ibjc]

latest findings from the ai audit!

Summary of Inverse Bonding contract:

1. Misleading Error Messages (Minor): This vulnerability may lead to confusion among users due to the shared error messages used in different contexts.

2. Improper Error Reporting (Major): This may cause unawareness of failure in case another contract reverts, hence continuing the execution of the smart contract leading to potential inconsistencies.

3. Incorrect Calculations (Severe): In the _calcMintToken function, inflated mint token value can be returned if the new amount added to the reserve balance exceeds the current

4. Missing Contract Versioning (Minor): Without versioning, it would be hard to distinguish between different versions of contracts during an upgrade situation.

5. Unsafe Typecast (Major): In function updateFeeConfig, ActionType actionType is converted to uint256 without checking ranges for the enum.

------- end of summary --------

The smart contract seems well-written and does not appear to contain any obvious vulnerabilities. It's worth noting that the contract uses well-established libraries such as OpenZeppelin for managing ERC20 tokens, as well as various security and access control features including SafeERC20, Ownable, Pausable, and Initializable.

It is important to be aware of some potential attack vectors that, while not necessarily explicit in this contract, can be common in Solidity contracts in general:

Misleading Error Messages: It is not a security issue per se, but it can cause users to misunderstand the contract. Some require statements share the same error messages, ERR_PARAM_ZERO and ERR_LIQUIDITY_TOO_SMALL, but are used in different contexts.

Improper Error Reporting The contract doesn't use revert or require to handle some of the failures appropriately. This is specifically problematic in methods that invoke functions on other contracts (like the token contracts' transfer, transferFrom, mint, and burn methods).

For instance, in the sellTokens function, the contract sends back some ETH to the seller:

(bool sent,) = recipient.call{value: returnLiquidity}("");
require(sent, ERR_FAIL_SEND_ETHER);

If the recipient is another contract, and that contract reverts in its fallback function, the sellTokens function would not be aware of this, and would continue execution, potentially leading to inconsistencies.

Incorrect Calculation The _calcMintToken function may return incorrect values. When amount added to the reserve balance exceeds the reserve balance, the newBalance becomes larger than _reserveBalance and thus the mint token could be drastically inflated.

Missing contract versioning: The contract does not contain versioning. In case of an upgrade, it will be hard to distinguish between different versions of contracts.

Mitigation: The code should include a versioning scheme. This could be a simple uint that is increased with every new deployed version.

Unsafe typecast: In function updateFeeConfig, ActionType actionType is being directly converted to uint256 without range check for the enum. This potentially could lead to a vulnerability if the function is called with invalid actionType.

Mitigation: Check if ActionType actionType actually corresponds to one of the defined enum items before converting it to uint256.