Spectre.io - MerkleProfitSharing - Security Review

Spectre_io_MerkleProfitSharing_Security_Review.md

MerkleProfitSharing - Security Review

Table of contest

• Introduction
• Repository
• MerkleProfitSharing.sol

Introduction

Spectre.io team requested the review of the contract under the repository MerkleProfitSharing referenced by the commit 5ac54557a97e084e374d48fb91004acbf1ffde0b: MerkleProfitSharing.sol.

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

MerkleProfitSharing.sol

Medium Severity

• M1 - line 198 - Do not depend on gas stipend - It is not recommended to use transfer() or send(). The recipient can be a smart contract and therefore if for any reason any involved OPCODE is priced, every ETH transfer will fail. You can read more about it here: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/. Consider using call instead within a re-entrancy prevention. Update: transfer has been changed to call along with a re-entrancy ward with the commit #628aee4f1e7e6733c386a2f8d238ee085733ff93. Consider adding it to the low level method _claim to reduce the contract bytecode.

• M2 - line 236 - Deposited amount mismatch - If _token is a deflationary token or a token where the amount received after a transfer differs from _amount, the balance of the smart contract after the transfer will be lower than the amount computed and therefore claims will likely to fail by throwing an overdrawn error. Consider checking the balance before and after the transferFrom to get the actual deposited amount. Update: Pre and post balance is being checked with the commit #628aee4f1e7e6733c386a2f8d238ee085733ff93.

Low Severity

• L1 - lines 201 and 236 - Lack of support for tokens without a return value - To be compatible with the smart contract, an allowed token should return true upon a successful transfer. This is not met in some implementations, like OMG (OmniseGO), BNB (Binance coin), and other not fully ERC20 compliant tokens. Consider using the Open Zeppelin SafeERC20 lib. You can read more about it here: https://decentraland.org/blog/technology/safe-erc20-transfers/. Update: SafeERC20 from openzeppelin has been added with the commit #628aee4f1e7e6733c386a2f8d238ee085733ff93.

• L3 - lines 276 and 298 - blacklist always add addresses by "turning off" previously blacklisted ones. This, based on the size of the blacklist, could end up running out of gas. Consider using only a mapping (addresses => boolean) and index/fetch blacklisted addresses by subscribing to events if it is going to be called on-chain.

Notes

• N1 - line 16 - Unused event Claimed. Update: The event has been removed with the commit #628aee4f1e7e6733c386a2f8d238ee085733ff93

• N3 - line 24 - Consider adding the keyword immutable for variables that won't change during the contract lifetime to reduce the gas cost of deploying the contract and when accessing them within a transaction.

• N4 - line 54 - Consider changing the name of onlyOwnerOrSpectreWalletOrLiquidityPool to onlyAuthorized and also add a mapping of authorized addresses for future use cases.

• N4 - lines 69, 73, 82, 99, 105, 119, 142, 146, 207, 229, 253, 274, 296 - Consider changing the visibility from public to external for functions that will be called only externally to reduce gas costs.

• N5 - lines 73 and 82 - getSxdtBalances and getSxdtClaims are functions that are not being used in the smart contract and can be easily moved offchain.

• N6 - lines 139, 151 - duplicated method - claim and claimOther end up calling _claim. One claim method receiving always the _claimer even if it is the sender, will simplify the contract and reduce the contract's state modifier windows. Funds are always being sent to the claimer's address as part of the Merkle tree node.

• N7 - line 147 - getNode doesn't return any node if the node is not "valid". Consider renaming the function to getValidNode or removing the checks by always returning a node.

• N8 - lines 182, 191, 194, 196, 201, 204 - Prevent loading multiple times state variables by storing them in memory whenever is possible to reduce gas consumption: dividends.

• N9 - line 250 - DividendDeposited event uses msg.sender as the account parameter but the tokens are being pulled from the depositor which can be different from the sender.

• N10 - line 263 - Consider using dividend from line 260 instead of the dividends mapping to reduce gas costs and also to make to code more readable.

• N11 - line 261 - Prevent transferring 0 - If remainingAmount is 0 the DividendReclaimed event can be directly emitted.

• N12 - lines 265 and 268 - Consider using the total balance of the contract in ETH or token respectively if the balance must be 0 after every reclaim.

Final Thoughts

• Radspec (dev notation): Consider adding dev notation to every function.

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

Message

• getSxdtClaims, _setLiquidityPool_, areClaimed, claim, are not being tested. Consider adding at least unit testing for each function.

• Not tested if liquidityPool can deposit Ether or ERC20 dividend.

• Not tested a re-entrancy attack

Ignacio Mazzara - 06/2021