Arbitrum (ARB) TraderDAOai Contracts Security Audit Report

ARB_TraderDAOai_Security_Audit.md

Arbitrum (ARB) TraderDAOai Contracts Security Audit Report.

1. Summary

TraderDAOai smart contracts security audit report performed by chhajershrenik.

2. In scope

Commit: f9361197082c833146120cb1a29c59c40fc11e8a

• Ambassador_Redeem_Contract.sol
• Liquidity_Wallet.sol
• POT_Token.sol
• Proof_Of_Trade_Arbi_One.sol

3. Findings

In total, 2 issues were reported, including:

• 2 critical severity issues.

In total, 26 notes were reported, including:

• 12 notes.

• 14 owner privileges.

3.1. Ambassador_Redeem_Contract

3.1.1. Owner privileges

Severity: owner privileges

Description

1. Contract Ambassador_Redeem_Contract contract inherits basic access control properties from Openzeppelin's Ownable contract, where the contract's ownership can be transferred or renounced. Renouncing ownership will leave the contract without an owner, thereby disabling any functionality that is only available to the owner.
2. Function SetPause() allows the owner to pause or resume the contracts functionalities available to the users, users would not be able to redeem USDT tokes for the signature signed by the signerAddress if the contract is paused.
3. Function SetSigner() allows the owner to update the signerAddress, all previous un-redeemed signatures will be invalid if the signerAddress is updated, new signatures must be generated for previous unclaimed signatures.
4. Function Save() allows owner to withdraw ERC-20 tokens from the contract.

3.2. Liquidity_Wallet

3.2.1. Function Claim() does not allow the trade of approved tokens

Severity: note

Description

ERC-20 standard allows the owner of tokens to approve a wallet address as a spender of the tokens. The spender cannot trade POT tokens using the function Claim() of the tokens which he is authorized to transfer.

Code Snippet

• https://github.com/TraderDAOai/contracts/blob/9147ba55e69c697090841f6a81fe4b3de3972aa6/Liquidity_Wallet.sol#L437-L451

Recommendation

Consider modifying the contract's logic to allow the authorized spender to trade POT tokens of a wallet.

3.2.2. Function SetDecimal() allows gov address to modify Decimal for conversion

Severity: note

Description

The function SetDecimal() allows the gov address to modify Decimal for the POT token used to value the POT token in USD. The POT token has a constant decimal of 18, modifying the decimal would lead to deflation/inflation in the value of the POT tokens.

Code Snippet

• https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/Liquidity_Wallet.sol#L429-L432

Recommendation

Consider removing the SetDecimal() function or making the decimals() call to the token contract to get the token decimals.

3.2.3. Owner privileges

Severity: owner privileges

Description

1. Contract Liquidity_Wallet contract inherits basic access control properties from Openzeppelin's Ownable contract, where the contract's ownership can be transferred or renounced. Renouncing ownership will leave the contract without an owner, thereby disabling any functionality that is only available to the owner.
2. Function SetPause() allows the owner to pause or resume the contracts functionalities available to the users, users would not be able to trade POT tokens for USDT tokens if the contract is paused.
3. Function SetGov() allows the owner to update the gov address.
4. Function Save() allows owner to withdraw ERC-20 tokens from the contract.

3.3. POT_Token.sol

3.3.1. The contract state cannot be paused

Severity: critical

Description

The contract uses the pause variable to determine whether the users can use contract functionalities, but the contract is missing a function to change the contract state.

Recommendation

Consider implementing a function to change the state of the contract by modifying the pause variable.

3.3.2. All functionalities except mint are available to the users when the contract state is paused

Severity: critical

Description

All ERC-20 token functionalities except function mint() is available to the user when the contract state is set to pause.

Recommendation

Consider implementing checks to restrict users from accessing functions if the contract state is paused based on the requirements.

3.3.3. Owner privileges

Severity: owner privileges

Description

1. Contract POT_Token contract inherits basic access control properties from Openzeppelin's Ownable contract, where the contract's ownership can be transferred or renounced. Renouncing ownership will leave the contract without an owner, thereby disabling any functionality that is only available to the owner.
2. Function ownerMint allows the owner to mint arbitrary tokens.
3. Function SetSigner() allows the owner to update the signerAddress, all previous un-redeemed signatures will be invalid if the signerAddress is updated, new signatures must be generated for previous unclaimed signatures.

3.4. Proof_Of_Trade_Arbi_One

3.4.1. Users Claim rewarded with USDT tokens

Severity: note

Description

Based on the docs the Proof_of_Trade_Arbi_One contract should rewards users in POT tokens, but the contract's implementation rewards users in USDT tokens.

Code Snippet

• https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/Proof_Of_Trade_Arbi_One.sol#L452

Recommendation

Consider reviewing the implementation or updating documentation based on the business requirements.

3.4.2. Owner privileges

Severity: owner privileges

Description

1. Function SetSigner() allows the owner to update the signerAddress, all previous un-redeemed signatures will be invalid if the signerAddress is updated, new signatures must be generated for previous unclaimed signatures.
2. Function SetPause() allows the owner to pause or resume the contracts functionalities available to the users, users would not be able to Deposit USDT tokens or Claim rewards if the contract is paused.
3. Function Save() allows owner to withdraw ERC-20 tokens from the contract.

3.5. Use of ecrecover with known vulnerabilities

Severity: note

Description

Contracts Ambassador_Redeem_Contract, POT_Token and Proof_of_Trade_Arbi_One uses soliditiy's ecrecover() function to recover signer address from the signature. The function ecrecover() is known to have signature malleability issues, therefore there can exist a valid alternative message hash with the same signature (v, r, and s) allowing an attacker to replay the signature. While the probability of an attacker reversing the alternative message hash and having access to the private key for the respective wallet address for the wallet in message data is highly unlikely, in case if possible it would allow the attacker to replay the signature to claim/mint arbitrary tokens based on the message.

The message hash generation also does not append any blockchain-specific information (PREFIX) to the message hash, allowing newly generated signatures to be replayed across both chains in case of a hard fork.

Code Snippet

• https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/Ambassador_Redeem_Contract.sol#L511-L527
• https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/POT_Token.sol#L662-L678
• https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/Proof_Of_Trade_Arbi_One.sol#L497-L513

3.6. Follow good coding practice

Severity: note

Description

1. Unlocked Pragma

Contracts should be deployed using the same compiler version/flags with which they have been tested. Locking the floating pragma, i.e. by not using ^ in pragma solidity ^0.8.16, ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs.

2. Missing docstrings

The contracts in the code base lack documentation. This hinders reviewers’ understanding of the code’s intention, which is fundamental to correctly assess not only security but also correctness. Additionally, docstrings improve readability and ease maintenance. They should explicitly explain the purpose or intention of the functions, the scenarios under which they can fail, the roles allowed to call them, the values returned, and the events emitted.

Consider thoroughly documenting all functions (and their parameters) that are part of the contracts’ public API. Functions implementing sensitive functionality, even if not public, should be documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

3. Missing test suite

The contract is missing a test suite to validate and verify the behavior of the contract functionalities. Add tests are recommended to ensure that the contract functions and behaves as expected.

4. Redundant comments

The contracts contain comments about files imported but were never imported in the contract.

• https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/Ambassador_Redeem_Contract.sol#L387
• https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/Liquidity_Wallet.sol#L394
• https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/Proof_Of_Trade_Arbi_One.sol#L389

5. Unused variable, mapping, and functions

The declared variable, mapping, and functions in the contracts are never utilized and can be safely removed to optimize gas costs during deployment.

• https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/Liquidity_Wallet.sol#L401
• https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/POT_Token.sol#L326
• https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/Ambassador_Redeem_Contract.sol#L462-L475
• https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/POT_Token.sol#L613-L626

6. Missing zero address checks

In several places in the code, addresses are passed as parameters to functions. In many of these instances, the functions do not validate that the passed address is not the address 0. While this does not currently pose a security risk, consider adding checks for the passed addresses being nonzero to prevent unexpected behavior where required, or documenting the fact that a zero address is indeed a valid parameter.

7. Insufficient event logging

Multiple functions with owner privileges in the contracts do not emit an event. Events are useful to inform external dapps or users that an important state was modified on the contract.

8. USDT variable can be marked as immutable and the function SetUSDT() can be removed

The Tether USD (USDT) contract is a proxy contract, it is very unlikely that the address would change as the contract can be upgraded by the respective team without changing the Tether USD (USDT) contract address. It would be safe to remove the function SetUSDT() from the contracts to optimize the contract code and save deployment gas costs. Marking the variable USDT as immutable in the contracts would also help save gas costs during variable reads.

4. Security practices

• Open-source contact.
• The contract should pass a bug bounty after the completion of the security audit.
• Public testing.
• Automated anomaly detection systems. - NOT IMPLEMENTED. A simple anomaly detection algorithm is recommended to be implemented to detect behavior that is atypical compared to normal for this contract. For instance, the contract must halt deposits in case a large amount is withdrawn in a short period until the owner or the community of the contract approves further operations.
• Multisig owner account.
• Standard ERC20-related issues. - PARTIALLY IMPLEMENTED. It is known that every contract can potentially receive an unintended ERC20-token deposit without the ability to reject it even if the contract is not intended to receive or hold tokens. As a result, it is recommended to implement a function that will allow extracting any arbitrary number of tokens from the contract.
• Crosschain address collisions. ETH, ETC, CLO, etc. It is possible that a transaction can be sent to the address of your contract at another chain (as a result of a user mistake or some software fault). It is recommended that you deploy a "mock contract" that would allow you to withdraw any tokens from that address or prevent any funds deposits. Note that you can reject transactions of native tokens deposited, but you can not reject the deposits of ERC20 tokens. You can use this source code as a mock contract: extractor contract source code. The address of a new contract deployed using CREATE (0xf0) opcode is assigned following this scheme keccak256(rlp([sender, nonce])). Therefore you need to use the same address that was originally used at the main chain to deploy the mock contract at a transaction with the nonce that matches that on the original chain. Example: If you have deployed your main contract with address 0x010101 at your 2021th transaction then you need to increase your nonce of 0x010101 address to 2020 at the chain where your mock contract will be deployed. Then you can deploy your mock contract with your 2021th transaction, and it will receive the same address as your mainnet contract.

3.2.1. Function Claim() does not allow the trade of approved tokens

It's not an issue.

Function Claim is designed to burn POT tokens a user has sent and return USDT to him. It shouldn't trade.

3.2.2. Function SetDecimal() allows gov address to modify Decimal for conversion

Decimals in this function is not connected with POT or USDT decimals directly. It's the DENOMINATOR for Rate. It uses in function Convert to convert POT token to USDT with some rate. With current values of Rate = 100 and Decimals = 10**18, the 1 POT = 0.0001 USDT

3.3.1. The contract state cannot be paused

It's low severity issue since it has a low impact on the contract. For example, the owner can still pause minting by changing the signerAddress to address(1).

3.3.2. All functionalities except mint are available to the users when the contract state is paused

It's a note.

There isn't a requirement for which functions of ERC20 should be paused. It's up to the developer's mind.

3.5. Use of ecrecover with known vulnerabilities

Signature Malleability means the same message can have two valid signatures.

So, signature (r,v,s) shouldn't use for identifying unique transactions. And in these contracts, only a message is used to check if the transaction has been processed.

Unlocked Pragma

It's not a good practice.

A good practice is: Contracts should be deployed using the same compiler version/flags with which they have been tested.

If pragma is unlocked by ^ it allows to compile using a newer version of the compiler that was not tested and may have some new issues.

• 3.2.1. It is not an issue and reported incorrectly.
• 3.2.2. Not enough info from developers, I assumed it was the decimals if the POT Token based on the value.
• 3.3.1 and 3.3.2. If that's the case then the state pause is unnecessary and can be removed. The docs and implementation of the state change and the respective restriction were missing in this case.
• 3.5. It was a typo from my end. Signature replay would be possible for same valid signature, which has different message hash. Although the likelihood of this happening is very low as the attacker would need to reverse the message hash and need to have the corresponding private key of the public key in the message hash.
• 3.6.1. Unlocked Pragma the recommendation suggests locking the solidity version.