travs
commented
5 years ago - [x] sellAndBurnMln() function of Engine contract the transfer of ETH is performed as msg.sender.send(ethToSend). However the return value is not checked as send() function returns bool. We recommend to use transfer() function, instead of send() for ETH transfer. This is a Low severity issue.
- [x] Theoretical integer underflow is possible in the makeOrder() function of PriceTolerance contract where this operation is performed int res = int(ratio) - int(_value). There is a SignedSafeMath library provided by OpenZeppelin
- [x] The takeOrder() function of EngineAdapter contract takes some addresses as parameter like targetExchange, orderAddresses [0] MLN token, orderAddresses [1] WETH token. These addresses can be faked by the Manager and would enable him to steal MLN tokens from the EngineAdapter contract.
[x] When performing the transfer out of the Vault (https://github.com/melonproject/protocol/blob/4e540de7bf84d1d1153327fc9e2e1e78a64e252b/src/contracts/fund/vault/Vault.sol#L14) you have no check whether the transfer actually worked. At first glance, it seems that in most places something else might fail (e.g. the token swap on Kyber) in case this transfer didn't work, but we were wondering if this is an assumption that you have. Otherwise, this could easily lead to the loss of tokens.
[x] The documentation says that the vault "provides extra security around ERC20 deposit and withdraw functionality, ... by ensuring that only the owner can withdraw from the vault when the vault’s locked state is false." (https://www.docs.melonport.com/chapters/fund.html#vaultsol) We couldn't find the "locked" variable and the "onlyUnlocked" modifier. Are we missing something?
[x] When transferring tokens you often have code such as: "require(ERC20(...).transfer(...)". Are you aware that this will not work with older tokens? Some older ERC20 tokens do not provide any return value when functions such as transfer() are called. Among, these tokens, there are some popular ones such as OmiseGo. This caused issues before: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca In your case, these tokens would get locked inside and could also block other tokens, e.g. when redeemWithConstraints() is called. (chainsecurity says they can suggest some possible usual solutions to this one)
[x] There are many contracts in the code which uses exponentiation operator (). The code is using Solidity 0.4.21 version which is prone to ExpExponentCleanup issue and its described in the known issues list https://etherscan.io/solcbuginfo . As per the issue description Using the operator with an exponent of type shorter than 256 bits can result in unexpected values.. This has been fixed into 0.4.25 Solidity version. https://blog.ethereum.org/2018/09/13/solidity-bugfix-release/ second comment: Just to clarify, pragma solidity ^0.4.21; makes sure that the used solc version to compile the contracts is at least version 0.4.21 or more recent. True, if you actually use 0.4.25 to compile them, you are not affected. However nothing prevents one to compile these contracts with the affected solc versions 0.4.21 - 0.4.24. We do recommend to change this to pragma solidity ^0.4.25 in order to prevent accidental compilations of the contracts using an older and affected solc version.