De'on Summers (github)[https://github.com/dsummers91]
De'on Summers, currently employed as the lead blockchain developer of Hashed Health for roughly two years, we work on a number of different blockchain protocols. Have worked as a contractor for BlockEra -auditing smart contracts on their behalf. Have been working with ethereum for over three years. Have done seven audits in total, four with BlockEra - Majority have been crowdsale contracts. Others include some state channel, and upgradability contracts. Developed on many languages such as solidity, vyper, LLL, and bamboo.
Trading.sol contains the logic to send trade requests. The current tests allows Kyber, Uniswap, Radar_Relay, and Bancor. A user would request a trade from Dex.ag, then the endpoint would populate data for the trading call that the user would execute in order to get the best price for a specific token trade.
The tokens are initially transfered to the Trading contract, then fees are token, and trade is sent to a specific DEX with the user receiving their requested token
The results from DEX API endpoint []
Since these results are dynamic from a central entity it is inferred that the data from this call are trusted. Steps should be taken to make sure this data will never be malicious.
When calcuting the fees to send to beneficiary and proveq the contract states
_transfer(toToken, beneficiary, feeAmount.mul(4).div(5), false);
_transfer(toToken, proveq, feeAmount.div(5), false);
The could result in a very small number of tokens being left over if the token amount is not divisible by 5. at most it would leave 4-e18 ether/tokens. But to be certain there are no tokens leftover I would recommend using calculating the fee for one entity, then using substraction to calclate whatever is leftover.
The external call method has safety concerns that should be addresses: Some safety concerns are:
The user is unable to know for sure what the results of the call may do, since hex calldata is sent to the contract
In order to interact with the contract users must approve an infinate amount of tokens for each token that will be traded (0 - 1 which equates to 2^256-1)
The calldata approves enough gas for a possible contract re-entrancy. But there is no malicious activity that could be done
Whitelisting contracts would give increased security to the contract. With variable external data, and infinite token approval given to the contract, a malicious contract would be able to transfer all of a users token to their wallet, whilst only giving the user the minimum tokens requested for the trade.
A contract could implement something as shown:
function malicious call(uint256 mintokensAmount, uint256 tokensAmount, IERC20 fromToken, IERC20 toToken, address from, :/) {
let tokensToSteal = tokensAmount
fromToken.transferFrom(msg.sender, address(this), tokensAmount)
msg.sender.transfer(toToken, minTokensAmount)
)
The only amount at risk would be the value difference between tokensAmount
In the past there has been clever attacks of cross-site scriptings or other forms of potential hacks, that a malicious actor can change certain information without the user knowing. In such instance, a malicious actor could send harmfull calldata to the transfer function, resulting in user losing their funds. In order to combat thing, it is recommended to sign (ECDSA signature) a hash of the calldata with a secure private key - in web3 this function is [web3.eth.sign], then within the contract it would verify that the signature using ecrecover, that the owned private key signed that data.