This document is a security audit report where sushiswap-settlement and bentobox have been reviewed.
- 7d80bae4926d212c3ffad050610cf19d ./OrderBook.sol
- 46cee707e6bb462f4affcfebe247fe9f ./Settlement.sol
- 14cb8ab1973dc2b1809f9c27909d4d8b ./UniswapV2Router02Settlement.sol
- 2d4ded3e2ab0d01d187a9457abefef8 ./interfaces/IMasterChef.sol
- 73382b325285432af6e89bef11dec9d5 ./interfaces/IMintable.sol
- 70ec2cf3a5ae0eb6cec673c4beb61cf5 ./interfaces/ISettlement.sol
- ba01b1c6046aab91923c57ccd3a15ef6 ./libraries/Bytes32Pagination.sol
- d0224a1a326b6cb83702c9c24c3d74c4 ./libraries/Orders.sol
- 2fcf13fc9f81c6ae97a1fe582c00f357 ./libraries/Verifier.sol
- 4763386b7f688e4963736e8e1580cc69 ./mixins/Ownable.sol
- bd2bdf9072e87301fd48881ea9bea180 ./ERC20.sol
- 7165e1bbc042e1bed9b8b10889ca339f ./libraries/BoringMath.sol
- 952bec4328cd74f10daea92087342bf7 ./libraries/Ownable.sol
- 1983e0e0a4e4175118835ffcfed40ca6 ./oracles/ChainLinkOracle.sol
- 1455dbd2c1a835d2d30a525592bb25f8 ./oracles/CompositeOracle.sol
- 89e038aebbced99e560822a942e0e5ed ./oracles/CompoundOracle.sol
- b900859dd29ca70501bc49e990e87795 ./oracles/PeggedOracle.sol
- 7f68aee95853cd72ec52cf2dcc4a4dd8 ./oracles/SimpleSLPTWAP0Oracle.sol
- 46b16592b7b561f61eafc3e74fe68220 ./BentoBox.sol
- cc8cfb53de3bd72365b53a6df6d35367 ./LendingPair.sol
- 69752817dbf62f3c2707936b02d2c400 ./swappers/SushiSwapSwapper.sol
- ./sushiswap-settlement/contracts/OrderBook.sol
- ./sushiswap-settlement/contracts/Settlement.sol
Offline signed orders submitted through OrderBook.createOrder or fillOrder._validateArgs do not implement EIP712. Without the correct usage of EIP712 the users will be prompted to sign a non-readable hexadecimal strings.
Such implementation represent a security risk:
- Users will need to sign unreadeable hexadecimal strings, which is confusing and insecure.
- Wallets that implement EIP712 such as Metamask combined with a correct EIP712 implementaion onchain, will also prevents signatures meant for one network, to work on another one. This feature will be enabled only if the chainId is used as a field EIP712 domain separator.
Fore more details refer:
- https://eips.ethereum.org/EIPS/eip-712
- https://docs.metamask.io/guide/signing-data.html#sign-typed-data-v4
- https://medium.com/metamask/eip712-is-coming-what-to-expect-and-how-to-use-it-bb92fd1a7a26
EIP712 must be implemented to avoid the issues described above.
- ./sushiswap-settlement/contracts/OrderBook.sol
OrderBook._addHash uses a loop to iterate over all the array of hashes to check the deadline. Knowing that OrderBook._addHash is used to append newly created order hash in OrdeoBook.createOrder into four different arrays, the gas consumption will increase depending on the array size untill reaching block gas limit. In the same function, moving all the array elmenents starting from the fetched index will also make the gas consumption extremly higher since it SLOAD SSTORE will be extensively used.
The actual sorting algorithm must be optimized or completely refactored.
- ./sushiswap-settlement/contracts/Settlement.sol
If an order maker decrease the allowace previously approved to the settlement contract the internal call to Settlement._swapExactTokensForTokens will throw. However, batch execution will also throw in this case, this behavior is not wanted by the calling addresses since it can cause them losses and delay when executing users orders that can lead to unexecuted orders if the price movement is too quick.
when executing multiple orders the allowance must be checked and the function must return amountOut equal to zero before transfering the tokens to the pair contract since the transaction will throw.
- ./sushiswap-settlement/contracts/mixins/ownable.sol
Every Solidity file specifies in the header a version number of the format pragma solidity (^)0.*.*. The caret (^) before the version number implies an unlocked pragma, meaning that the compiler will use the specified version and above, hence the term "unlocked".
For consistency and to prevent unexpected behavior in the future, it is recommended to remove the caret to lock the file onto a specific Solidity version.
- ./sushiswap-settlement/contracts/mixins/ownable.sol
The clone-and-own approach involves copying and adjusting open source code at one's own discretion. From the development perspective, it is initially beneficial as it reduces the amount of effort. However, from the security perspective, it involves some risks as the code may not follow the best practices, may contain a security vulnerability, or may include intentionally or unintentionally modified upstream libraries.
Rather than the clone-and-own approach, a good industry practice is to use the Truffle framework for managing library dependencies. This eliminates the clone-and-own risks yet allows for following best practices, such as, using libraries.
- ./bentobox/contracts/ERC20.sol
Following EIP-20, ERC20 functions transfer and transferFrom must allow zero value transfers and should throw in case if the users has insufficient balance or allowance. The implemented ERC20 contract does not comply with the description stated above.
Follow the ERC20 specifications to implement a fully compatible ERC20 token.
Externaly signed order are not protected against signature malleability. No unique value (such as nonce for each user) is saved on chain to guarentee the uniqueness of the signature.
For more details refer to https://swcregistry.io/docs/SWC-117.
- ./sushiswap-settlement/contracts/OrderBook.sol
- ./sushiswap-settlement/contracts/Settlement.sol
V, R, S signature inputs are not validated, all values should be verified as recommended here.
- ./bentobox/contracts/ERC20.sol
The hardcoded TYPE_HASH value used in ERC20.permit function cannot be verified.
Either calculate the TYPE_HASH on chain or include the hashed string in the code as a comment.
- ./bentobox/oracles/ChainLinkOracle.sol
The return value from IAggregator.latestRoundData is not checked if stale, please note that the returned values in get and peek are hardcoded to be true. ChainLink oracle relies on third paties to update the aggregator average price.
If the project team cannot guarentee that the aggragator is updated, the value returned by get and peek must return false when the aggregator price is stale.
- ./bentobox/oracles/ChainLinkOracle.sol
_get function uses a decimals value passed throught function arguments, the decimals value might be incorrect since every Chainlink aggregator uses it own decimals value.
Chainlink aggregator implement a function that returns the decimals value of the returned average price, the value should be used to compute the final price in _get function.
- ./bentobox/contracts/LendingPair.sol
Exchange rate is not updated when isSolvent is callad, under colateralized users might be able to borrow asset even if they should not in case of a correct exchange rate.
Exchange rate should be updated prior to such actions.
- ./bentobox/contracts/BentoBox.sol
If a user owns WETH in his EOA he won't be able to deposit it since the BentoBox.deposit function handle WETH as if it was ETH, meaning that the WETH.deposit external call will fail. Simialrly a user who wants to withdraw WETH will get ETH instead. The last one can be problematic for contract that does not handle such situaltion.
A dummy ETH address can be used to handle ETH deposit and withdrawal that is not equal to the WETH token address.
- ./bentobox/contracts/BentoBox.sol
skim/skimTo allow anyone to take tokens deposited through transfer function directly.
Use access restriction to allow only required contracts and administrators to call skim/skimTo
- ./bentobox/oracles/CompoundOracle.sol
In _getPrice and _peekPrice blocknumber + 8 is always higher than info.blocknumber. This will lead to price to be updated at every call.
Set info.blocknumber to block.number + 8, and change the condition to if (block.number > info.blockNumber).
- ./bentobox/oracles/CompoundOracle.sol
It is undertermined if the Compound anchor returned price decimals fixed. However, in peek and get functions 1e36 is used as base.
Decimals should be get dynamicaly from their related contracts or tokens to be sure that no conversion errors happen.