Audit done by Saksham
https://github.com/TraderDAOai/contracts/tree/f9361197082c833146120cb1a29c59c40fc11e8a
Some tokens (like USDT) don’t correctly implement the EIP20 standard and their transfer/transferFrom functions return void, instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert.
Affected instances:
Ethereum uses elleptic curve cryptography. The problem that arises with signatures is that for every signature that exists on
the elleptic curve , there exists a similar signature on the other side of the curve , this would mean a single signature
holds true twice on the curve.
To tackle this we eliminate one side of the curve , OpenZeppelin performs this https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L134
to eliminate malleability or replay attack by restricting the s value.
As the current code here https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/POT_Token.sol#L662-L677 does not check for this ,
OZ's implementation of the ECDSA should be used to prevent signature malleability.
The Decimal value is 1e18 here https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/Liquidity_Wallet.sol#L403 , This value is used to calculate return amount of USDT here https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/Liquidity_Wallet.sol#L445 . Since USDT has 6 decimals , this calculation would be incorrect and the resulting return amount would be 1/1e12 lesser.
Remediation would be to use 1e6 as the value for Decimals.
When the contract is paused , the transfers should be blocked too as the contract is meant to be paused. Therefore , the
function here https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/POT_Token.sol#L394 should check if the pause = false .
When the contract is paused , burning should be blocked too as the contract is meant to be paused. Therefore , the
function here https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/POT_Token.sol#L502 should check if the pause = false .
The decimal value if changed will give a different amount , it should always be fixed .
Affected instance:
The owner can change the address to the USDT here https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/Ambassador_Redeem_Contract.sol#L404 , this functionality is dangerous moreover there should not exist a functionality where one can change the address of let's say USDT as it should be a constant.
Mint functionality is supposed to be blocked when the contract is paused , but the owner can mint from here https://github.com/TraderDAOai/contracts/blob/f9361197082c833146120cb1a29c59c40fc11e8a/POT_Token.sol#L494 . This function should also check if the contract is paused or not.
The current owner can update all the state variables like here:
Ensure that the owner is a multisig.
Instead of including all the code from openzepplin into the contracts we should just simply use import and make the code more cleaner and readable. Including all the code makes the contract unnecessarily big and complex.
If events are emitted before sending funds , it introduces the possibility that some events could be emitted out of order if the recipient’s fallback function executes another operation. Consider emitting the event before the funds transfer.
Affected instances:
Many functions 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 clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
The code base should always have a test suite to test out all the functionality and ensure a 100% converage of the codebase.
abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
If all arguments are strings and or bytes, bytes.concat() should be used instead
Instances (3):
File: Ambassador_Redeem_Contract.sol
525: bytes32 message = keccak256(abi.encodePacked(_word));
File: POT_Token.sol
676: bytes32 message = keccak256(abi.encodePacked(_word));
File: Proof_Of_Trade_Arbi_One.sol
511: bytes32 message = keccak256(abi.encodePacked(_word));
Instances (6):
File: Ambassador_Redeem_Contract.sol
411: signerAddress = _address;
File: Liquidity_Wallet.sol
434: gov = _address;
File: POT_Token.sol
183: _owner = msgSender;
718: signerAddress = _address;
File: Proof_Of_Trade_Arbi_One.sol
430: signerAddress = _address;
433: ByBitAddress = _address;
Instances (3):
File: Ambassador_Redeem_Contract.sol
493: require(sig.length == 65);
File: POT_Token.sol
644: require(sig.length == 65);
File: Proof_Of_Trade_Arbi_One.sol
479: require(sig.length == 65);
Instances (7):
File: Ambassador_Redeem_Contract.sol
443: bytes memory s = new bytes(40);
445: bytes1 b = bytes1(uint8(uint(uint160(x)) / (2**(8*(19 - i)))));
464: bytes memory bytesArray = new bytes(64);
File: Liquidity_Wallet.sol
333: bstr[k--] = bytes1(uint8(48 + _i % 10));
File: POT_Token.sol
615: bytes memory bytesArray = new bytes(64);
File: Proof_Of_Trade_Arbi_One.sol
458: bytes memory s = new bytes(40);
460: bytes1 b = bytes1(uint8(uint(uint160(x)) / (2**(8*(19 - i)))));
Instances (24):
File: Ambassador_Redeem_Contract.sol
99: function owner() public view returns (address) {
125: function renounceOwnership() public onlyOwner {
134: function transferOwnership(address newOwner) public onlyOwner {
432: function toBytes(address a) public pure returns (bytes memory b){
462: function bytes32ToString(bytes32 _bytes32) public pure returns (string memory) {
477: function stringToBytes32(string memory source) public pure returns (bytes32 result) {
File: Liquidity_Wallet.sol
88: function owner() public view returns (address) {
114: function renounceOwnership() public onlyOwner {
123: function transferOwnership(address newOwner) public onlyOwner {
File: POT_Token.sol
209: function renounceOwnership() public onlyOwner {
218: function transferOwnership(address newOwner) public onlyOwner {
448: function increaseAllowance(address spender, uint256 addedValue) public returns (bool) {
467: function decreaseAllowance(address spender, uint256 subtractedValue) public returns (bool) {
480: function mint(string calldata _rawdata,bytes calldata _sig, uint256 amount) public returns (bool) {
494: function ownerMint(uint256 amount) public onlyOwner returns (bool) {
502: function burn(uint256 amount) public returns (bool) {
598: function toBytes(address a) public pure returns (bytes memory b){
613: function bytes32ToString(bytes32 _bytes32) public pure returns (string memory) {
628: function stringToBytes32(string memory source) public pure returns (bytes32 result) {
File: Proof_Of_Trade_Arbi_One.sol
100: function owner() public view returns (address) {
126: function renounceOwnership() public onlyOwner {
135: function transferOwnership(address newOwner) public onlyOwner {
412: function Deposit(uint amount, uint tradetype , string memory id) public{
516: function toByte(uint8 _uint8) public pure returns (bytes1) {
If not cached, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first). If run for enough times the loop would waste alot of gas.
Example :
Signature Malleability Attack
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.
It's not an issue here.