Skip to content

Instantly share code, notes, and snippets.

@nachomazzara

nachomazzara/Tradestars_Token_And_Card_Security_Review.md

Last active April 14, 2021 00:21
Show Gist options

  • Select an option

    Learn more about clone URLs
  • Save nachomazzara/46fd301d8a3741ab02c1bf5585dec686 to your computer and use it in GitHub Desktop.

Select an option

Learn more about clone URLs
Save nachomazzara/46fd301d8a3741ab02c1bf5585dec686 to your computer and use it in GitHub Desktop.
Download ZIP
Tradestars - Token & Card - Security Review
Raw
Tradestars_Token_And_Card_Security_Review.md

Tradestars - Security Review

Table of contest

Introduction

The Tradestars team requested the review of the contracts under the repository tradestars-token referenced by the commit 73c30958493004434ad6272765132b91749be41c: Vesting.sol, TransferWithSigERC20.sol, TSX.sol, sTSX.sol, sTSXChild.sol , SafeDelegate.sol, Relayer.sol, UniswapManager.sol and Cashier.sol. . And the repository tradestars-cards references by the commit 948abf1a7f4964c4cda0d4f9412f4ad37cef6867: Administrable.sol, GasPriceLimited.sol, ERC20Manager.sol, FractionableERC721.sol, PerformanceCard.sol and BondedERC20.sol

The rest of the contracts in the repositories are assumed to be audited.

tradestars-token

The tests can't be run with the current setup. After installing some missing dependencies and set up everything the test run, but Withdraw tests -> withdraw OK fails.

TokenVesting.sol

Low Severity

  • L1 - line 147 - Can't rescue tokens by owner - If a token has been revoked and the contract receives more of that token, the owner of the contract won't be able to rescue them. Only the beneficiary will be able to transfer them out. Consider allowing the owner of the contract to always rescue tokens if the token is revoked.

Notes

  • N1 - lines 29, 32, 33, 34 and, 36 - Consider adding the keyword immutable for variables that wont't change during the contract lifetime to reduce the gas cost of deploying the contract and when access them within a transaction.

  • N2 - lines 78, 85, 92, 99, 106, 113, 120, 128, and 145 - Consider changing the visibility from public to external for functions that will be called only externally to reduce gas costs. When the function is external parameters are reading from calldata instead of copying in memory which is way cheaper.

TSX.sol

Notes

  • N1 - line 26 - Consider changing the visibility from public to external for functions that will be called only externally to reduce gas costs.

  • N2 - line 51 - Reduce gas costs on errors - If the token is paused, the transfer will fail. Consider moving the require of whether the token is paused to either at the beginning of the transfer functions or at the beginning of _beforeTokenTransfer.

TransferWithSigERC20.sol

High Severity

  • H1 - line 40 - Replay of signatures on different chains - The chain id is hardcoded with the Matic Mumbai value at Smart Contract EIP712.sol (80001). If the contract is deployed in another chain with the same address, the same message can be replayed. Consider loading the chainId by using the opcode chainid.

  • H2 - line 57 - Malicious relayer - Although the relayer is part of the signature, the destinatary of the funds is not. A malicious relayer can use this to relay a transfer transaction and send the funds to himself. Consider adding the to parameter to the message to be signed.

LibTokenTransferOrder.sol

Notes

  • N1 - line 16 - Unused struct - TokenTransferOrder is not being used. Consider removing the unused struct.

  • N2 - Consider adding dev notation to every function and modifier.

sTSX.sol

Notes

  • N1 - line 32 - No event emission - Consider emitting an event when minter is set to track them off-chain in case of being helpful.

  • N2 - line 32 - Limited roles - The minter of the contract may be another contract. Contracts may need an upgrade in case of a vulnerability or a feature need. Consider using a mapping instead of a single value to prevent being limited in case of more than one minter is necessary.

  • N3 - lines 32 and 42 - Consider changing the visibility from public to external for functions that will be called only externally to reduce gas costs.

  • N4 - line 42 - approveOperator is risky and controvert. Moreover, is not a standard method of an ERC20. This method is being used by the owner to set allowances to other Smart Contracts on the Cashier.sol behalf. But can be used to set allowances to anyone on behalf of anyone. Consider using a method directly on the Cashier to accomplish the same result.

  • N5 - Consider adding dev notation to every function and modifier.

sTSXChild.sol

Medium Severity

  • M1 - line 44 - Can't send meta transactions for all the functions - Meta transactions are supported in the contract by using the Relayer.sol Smart Contract. The token inherits from the ERC20.sol Smart Contract implementation from Open Zeppelin. The standard functions increaseAllowance, decreaseAllowance, transfer, etc. use the _msgSender function from the Context.sol Smart Contract which basically returns msg.sender. Users are not going to be able to send meta transactions for the standard functions. Consider using _getSafeRelayedSender in the standard functions or redefine _msgSender with the _getSafeRelayedSender implementation.

Low Severity

  • L1 - line 44 - Meta transactions are supported in the contract by using the Relayer.sol Smart Contract. depositorRole is being compared against msg.sender where the sender of the transaction could be a relayer. Consider using _getSafeRelayedSender to allow meta transactions for deposits too

Notes

  • N1 - line 31 - No event emission - Consider emitting an event when depositorRole is set to track them off-chain in case of being helpful.

  • N2 - line 31 - Consider changing the visibility from public to external for functions that will be called only externally to reduce gas costs.

  • N3 - line 31 - Limited roles - The depositor of the contract may be another contract. Contracts may need an upgrade in case of a vulnerability or a feature need. Consider using a mapping instead of a single value to prevent being limited in case of more than one minter is necessary.

SafeDelegate.sol

Notes

  • N1 - Consider adding dev notation to every function and modifier.

Relayer.sol

High Severity

  • H1 - line 35 - Replay of signatures on different chains - Due to the lack of checking on which chain the message was signed, the same message can be replayed on different EVM compatible chains. Consider adding the chainId to the message to be signed. Also, it is a good practice to add the address of the recipient contract.

Notes

  • N - line 36 - Unforced nonce - The nonce has a weak relation. Users can jump from using nonce 1 to N without being consecutive. Consider storing it to enforce an auto incremental execution of them.

UniswapManager.sol

Nothing found.

Cashier.sol

High Severity

  • H1 - line 76 - sTSX can be moved outside - A user can call withdraw with a _tokenAmount_ value different from the amount to be unlocked by the exit call on the posBridge. If that happens, withdrawAmount will be grater than 0 and the user's balance of _bridgeableToken will be greater than 0 too. Consider checking if the user's balance after the exit call is equal to tokenAmount to prevent the transfer out of sTSX tokens.

  • H2 - line 117 - Front-running - A user calls withdraw with all the parameters needed to perform a transfer of the sTSX token from his account to this contract. This transfer is relayed by this contract. The user has to previously signed a message for so. The amount being transferred is expected to be the same amount of sTSX tokens unlocked by the exit call which happens immediately before. Once the sTSX tokens are burnt, the amount burnt (withdrawAmount) is being transferred to the msg.sender in the desired _paymentToken. As there is not check between who is doing the transaction (msg.sender) and the user who has signed the message needed to do the transfer of the unlocked sTSX to the contract, another user can see the Mempool and do the same transaction with more gas price in order to be included first ending up with the steal of _paymentToken. Consider transferring the _paymentToken amount to the one who has signed the messages and who is the beneficiary of the unlocked tokens.

  • H3 - line 179 - Double minting - When a user deposits a token, _depositAmount of bridgeableToken is being minted to the contract. Later, _posDeposit is called and _depositAmount of bridgeableToken is being minted again to the contract. Consider removing the bridgeableToken mint at deposit and keep only the _postDeposit which is also used by Ether's deposits.

Low Severity

  • L1 - line 160 - Deposited amount mismatch - If depositedToken is a deflationary token or a token where the amount received after a transfer differs from _depositAmount, the swap from that token to the reserveToken will fail because the input amount to be swapped is not going to match the balance of the contract after the safeTransferFrom. Consider checking the balance before and after the safeTransferFrom to get the actual deposited amount.

Notes

  • N1 - lines 47, 49 and 50 - Consider adding the keyword immutable for variables that wont't change during the contract lifetime to reduce the gas cost of deploying the contract and when access them within a transaction.

  • N2 - line 61 - No event emission - Consider emitting an event when swapManager is set to track them off-chain in case of being helpful.

  • N3 - lines 86, 94, 98, 105 and 125 - Prevent loading multiple times state variables by storing them in memory or by changing it to immutable whenever is possible to reduce gas consumption: bridgeableToken and reserveToken.

  • N4 - line 128 - There is a comment with a question mark /// maxDstAmount ??.

  • N - The Tradestars team mentioned a requirement of stacking the reserveToken into a Defi protocol. The current implementation of the contract has no ability to transfer out the reserve tokens from the user's deposits. Consider adding a new function to perform a transfer out or stacking the reserves directly in a Defi protocol knowing their own risk.

tradestars-cards

Administrable.sol

Notes

  • N1 - Consider adding dev notation to every function and modifier.

GasPriceLimited.sol

Notes

  • N1 - Consider adding dev notation to every function and modifier.

  • N2 - Nowadays the gas price is too volatile. It is not recommended to limit the gas price to be used. Chances are that administrators will constantly change this variable. Also, until an administrator changes this value, all the transactions will fail or will be pending forever.

ERC20Manager.sol

Notes

  • N1 - Consider adding dev notation to every function and modifier.

  • N2 - line 17 - Consider returning directly the result of the new BondedERC20 tokens instead of storing it first in memory to reduce gas consumption.

FractionableERC721.sol

Notes

  • N1 - line 50 - Limited roles - The token manager of the contract may be another contract. Contracts may need an upgrade in case of a vulnerability or a feature need. Consider using a mapping instead of a single value to prevent being limited in case of more than one minter is necessary.

  • N2 - lines 50, 58, 67, 117, 142, 161, 185, 209 and 236 - Consider changing the visibility from public to external for functions that will be called only externally to reduce gas costs.

  • N3 - lines 50 and 58 - No event emission - Consider emitting an event when tokenManager and bondedHelper are set.

  • N4 - line 74 - Useless require - If the BondedERC20 is not set, the address will be the zero one and therefore the call to set the reserve ratio are going to fail. Also, the storage for fungiblesMap[_tokenId] is being loaded twice. To reduce gas consumption consider storing the result in memory the first time.

  • N5 - line 185 - amount is being used for the amount of tokes to be minted. Consider changing the name of the method from estimateBondedERC20Tokens to estimateBondedERC20Amount as estimateBondedERC20Value is already there.

  • N6 - line 200 and 229 - Reduce gas consumption by returning the result of the function call instead of storing it in memory.

  • N7 - lines 217 and 223 - Useless check and excessive gas consumption - The total supply of the bonded erc20 is being fetched twice. Also, the check of totalSupply being greater or equal to the input amount is being done at BondedERC20Helper.sol line 207.

PerformanceCard.sol

High Severity

  • H1 - line 79 - executeRelayedTx has the same issues as it has in the Relayer.sol. Evaluate using the same lib for both contracts.

  • H2 - lines 189 and 323 - Possible front-running - Although createCard and purchase have a gas price limit modifier, a user can send the same transaction with the same parameters and more gas price, if the maximum gas price was not set, to receive the BondedERC20 tokens without paying for them. Consider using the address of the reserveTokens payer instead of using the msgSender() for the beneficiary of the BondedERC20 tokens.

Medium Severity

  • M1 - line 220, 315, and 401 - Mismatch of expectations - A user can send a transaction to do a BondedERC20 swap or purchase expecting an amount in exchange. This expected amount may be fetched before sending the transaction by using the estimateBondedERC20*, estimateSwap, or estimatePurchase functions. That means that once the transaction is sent the expected result could not be the same ending up with fewer BondedERC20 tokens. Consider passing an expected minimum BondedERC20 amount for _destTokenId (swap) and _tokenId (purchase). Also, this may happen at the liquidate function. Consider adding a check for the reserveToken amount expected in return for liquidating BondedERC20 tokens.

Low Severity

  • L1 - line 171 and 294 - Unforced balance checker - The sender's balance of reserveToken is being checked before calling transferWithSig but this sender is not necessarily the same that signed the transfer. Also, if from has not enough balance, the transaction will fail because of the checks on the ERC20 token. Consider removing this require to save gas.

  • L2 - lines 308, 346, 397 and 430 - 0 fee for lower payment amount - If paymentAmount is lower than or equal to 199, pFee will be 0. Consider increasing pFee to 1 if the fisrt math returns 0.

  • L3 - line 265 - estimateSwap should use _destTokenId instead of _tokenId to calculate the slippage.

Notes

  • N1 - line 43 - Consider making PLATFORM_CUT and/or ERC20_INITIAL_SUPPLY editable by a governance mechanism chosen by the community.

  • N2 - line 66 and 69 - Reduce gas consumption - Consider adding the keyword immutable for variables that wont't change during the contract lifetime to reduce the gas cost of deploying the contract and when access them within a transaction.

  • N3 - lines 148, 203, 241, 286, 338, 375, and 416 - Consider changing the visibility from public to external for functions that will be called only externally to reduce gas costs.

  • N4 - line 150 - Useless require - If the BondedERC20 token was deployed, it will fail when trying to mint the ERC721 token again. Consider moving all the necessary requirements to the contract who has the responsibility to deploy them (FractionableERC721.sol) instead of this one.

  • N5 - line 161 - Useless parameter - The checkHash is the message hash and is being generated by the function parameters. Passing the _msgHash is not being useful as the message generated should be already signed by an admin. Consider removing the _msgHash parameter.

  • N6 - lines 170, 173, 275, 277, 293, 296, 330, 367, 403, and 408 - Wrong comment - sTSX should be sTSXChild.

  • N7 - lines 205 and 210 - Useless require - If the BondedERC20 tokens for _tokenId and/or _destTokenId have not been deployed, the call to the zero address will fail. Consider removing these require to reduce gas consumption.

  • N8 - lines 244, 265, 357 and 423 - Excesive gas consumption - The address of the bonded ERC20 is being fetched twice. Consider saving the result of the first fetch in memory to save gas.

  • N9 - lines 206, 211, 215, 220, 226, 227, 244, 249, 254, 260, 265, 289, 294, 297, 301, 309, 315, 321, 341, 352, 378, 383, 389, 398, 404, 419, 423, and 424 - Excesive sloads - save variables to memory to prevent loading them from the state multiple times or change them to immutable: nftRegistry, reserveToken

BondedERC20.sol

Notes

  • N1 - line 33 - Reduce gas consumption - Consider adding the keyword immutable for variables that wont't change during the contract lifetime to reduce the gas cost of deploying the contract and when access them within a transaction.

  • N2 - line 48 - No event emission - Consider emitting an event when reserveRatio is set.

Final Thoughts

  • Multiple mechanisms of meta-transactions: The sTSXChild token has multiple meta transaction mechanisms, one specific for transfers and the othe for any kind of transaction. Consider use only one to avoid human errors, open hacking windows, and to allow a better debugging traceability.

  • Tests: Consider adding more tests for every function along with testing meta transactions.

  • Consider allowing meta-transactions for the BondedERC20.sol, FractionableERC721.sol and every smart contract that will be deployed in an EVM compatible chain rather than Ethereum.

Ignacio Mazzara - 04/2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment