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.
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.
- 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.
-
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
toexternal
for functions that will be called only externally to reduce gas costs. When the function isexternal
parameters are reading fromcalldata
instead of copying in memory which is way cheaper.
-
N1 - line 26 - Consider changing the visibility from
public
toexternal
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
.
-
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 opcodechainid
. -
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.
-
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.
-
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
toexternal
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.
- 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 returnsmsg.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.
- L1 - line 44 - Meta transactions are supported in the contract by using the Relayer.sol Smart Contract.
depositorRole
is being compared againstmsg.sender
where the sender of the transaction could be a relayer. Consider using_getSafeRelayedSender
to allow meta transactions for deposits too
-
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
toexternal
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.
- N1 - Consider adding dev notation to every function and modifier.
- 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.
- 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.
Nothing found.
-
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 theexit
call on theposBridge
. If that happens,withdrawAmount
will be grater than0
and the user's balance of _bridgeableToken
will be greater than0
too. Consider checking if the user's balance after theexit
call is equal totokenAmount
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 theexit
call which happens immediately before. Once the sTSX tokens are burnt, the amount burnt (withdrawAmount
) is being transferred to themsg.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
ofbridgeableToken
is being minted to the contract. Later,_posDeposit
is called and_depositAmount
ofbridgeableToken
is being minted again to the contract. Consider removing thebridgeableToken
mint atdeposit
and keep only the_postDeposit
which is also used by Ether's deposits.
- 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 thereserveToken
will fail because the input amount to be swapped is not going to match the balance of the contract after thesafeTransferFrom
. Consider checking the balance before and after thesafeTransferFrom
to get the actual deposited amount.
-
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
andreserveToken
. -
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.
- N1 - Consider adding dev notation to every function and modifier.
-
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.
-
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.
-
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
toexternal
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
andbondedHelper
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 fromestimateBondedERC20Tokens
toestimateBondedERC20Amount
asestimateBondedERC20Value
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.
-
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
andpurchase
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 thereserveTokens
payer instead of using themsgSender()
for the beneficiary of the BondedERC20 tokens.
- 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
, orestimatePurchase
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 theliquidate
function. Consider adding a check for thereserveToken
amount expected in return for liquidating BondedERC20 tokens.
-
L1 - line 171 and 294 - Unforced balance checker - The sender's balance of
reserveToken
is being checked before callingtransferWithSig
but this sender is not necessarily the same that signed the transfer. Also, iffrom
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 to199
,pFee
will be 0. Consider increasingpFee
to1
if the fisrt math returns0
. -
L3 - line 265 -
estimateSwap
should use_destTokenId
instead of_tokenId
to calculate the slippage.
-
N1 - line 43 - Consider making
PLATFORM_CUT
and/orERC20_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
toexternal
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 theserequire
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
-
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.
-
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.