Skip to content

Instantly share code, notes, and snippets.

@nachomazzara

nachomazzara/Spectre_io_HODLVaultSeviceContract_Security_Review.md

Last active September 13, 2021 13:13
Show Gist options
  • Save nachomazzara/d460c3dacecbe9081709cd95f7a4973c to your computer and use it in GitHub Desktop.
Save nachomazzara/d460c3dacecbe9081709cd95f7a4973c to your computer and use it in GitHub Desktop.
Download ZIP
Spectre.io - HODLVaultSeviceContract - Security Review
Raw
Spectre_io_HODLVaultSeviceContract_Security_Review.md

HODLVaultSeviceContract - Security Review

Table of contest

Introduction

Spectre.io team requested the review of the contract under the repository HODLVaultSeviceContract referenced by the commit bb51ac9001b4316562601afc6415210b1bc196cc: HODLVaultSeviceContract.sol.

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

HODLVaultSeviceContract.sol

High Severity

  • H1 - line 310 - Admin index loss - Once an admin is about to be removed, the last admin in hodlAdminsAddressList is being used to replace the place of the admin to be removed. Then, the last admin is being popped from hodlAdminsAddressList because it is already in the place (index) of the removed admin. The value of the removed admin in hodlAdminsAddressListIndices is being cleared with 0 but the last admin's value used to replace its place is not updated. This will end up with wrong indices, admins that can't be removed, and/or undesired admins being removed. Consider updating the index value of the admin used to override in hodlAdminsAddressListIndices to keep the indices up to date. Update: The team fixed it on 0b1d477e7fec56db570ca41fefe6345176c534ef by updating the index value fo the admin used to override.

  • H2 - line 375 - Deposited amount mismatch - If tokenAddress 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 different than the amount and fees computed. Therefore, users may not be able to withdraw their deposits. Consider checking the balance before and after the transferFrom to get the actual deposited amount, and calculate the fees. Update: The team fixed it on 0b1d477e7fec56db570ca41fefe6345176c534ef but added a new vulnerability where after calling _calculateDepositFee_ for the second time, accept is not being checked. Also calling calculateDepositFee twice is not necessary.

  • H3 - line 382, 385, 410, 465, 482, 487 - Possible critical overflow - Even as it is unlikely to happen if a tokenAddress reaches the maximum uint possible for its accumulative fees, will occur an overflow resetting its value starting from 0 again. The same may happen with depositCount and/or withdrawalCount where if an overflow occurs, deposits and/or withdraws will have the same id, ending up with old ones being overridden by new ones. Moreover, tokenWiseDepositValue, and penaltyFeesTokenBalances[deposit.token] will endup with the same issue breaking bonuses calculation. Consider using SafeMath to prevent possible overflows. Update: The team fixed it on ced8378b1424d99b33c01f2a351902ac7bb3019d but the usage of SafeMath is no longer necessary as Solidity 0.8 comes with default underflow and overflow checks

Medium Severity

  • M1 - lines 257 - In the constructor of the contract, the admin is not added to the hodlAdminsAddressListIndices creating inconsistencies and different issues when trying to remove admins. See N4 for a way to solve this. Update: The team fixed it on 0b1d477e7fec56db570ca41fefe6345176c534ef by adding an internal function to add admins.

  • M2 - lines 274 and 304 - Possible loss of fee collector - If by a human mistake the new fee collector is the ZERO address, then no one will be able to recover it. Some ERC20 contracts prevent transferring to the ZERO address blocking withdrawals from old and further deposits. Following this vector, if the feeCollector is the ZERO address and there is only one admin different from the ZERO address, that admin can call removeAdmin using msg.sender equal to admin to remove themself from the admin list ending up with no admins and therefore without a way to set new ones. Update: The team fixed it on 0b1d477e7fec56db570ca41fefe6345176c534ef by preventing the newFeeCollector to be the ZERO address.

  • M3 - line 289 - The same admins can be added more than once. This will create inconsistencies and different issues when trying to remove admins. Consider checking if the admin about to be added was already added. Update: The team fixed it on 0b1d477e7fec56db570ca41fefe6345176c534ef by checking if the admin about to be added is already an admin. Take note that there is a typo in the require message lready instead of already.

  • M4 - line 476 - By using flash loans, the user that is withdrawing a deposit can have balance of SXUTToken equal or higher than bonusEligibleSXUTMinBalance. Consider using balanceOfAt if SXUTToken supports balance snapshots or any other strategy to check that the user is a legit SXUTToken holder. Update: The team fixed it on 0b1d477e7fec56db570ca41fefe6345176c534ef by using balanceOfAt. Take note that with just the previous block will work too.

Low Severity

  • L1 - line 280 - The new fee collector is being added as an admin in _hodlAdmins_ but not added to hodlAdminsAddressList. The feeCollector as admin could not be removed. Consider adding the new fee collector to the list too in order to keep the consistency of the contract variables. Plus: see N4 for a better approach.

  • L3 - lines 379, 503, and 506 - 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: The team fixed it on ced8378b1424d99b33c01f2a351902ac7bb3019d by using using the Open Zeppelin SafeERC20 lib. Consider using it too for depositToken.transfer(feeCollector, depositFee).

  • L3 - line 432 - Inefficient and risky storage update - The deposit is being copied and modified in memory to be "saved" with all of the changes based on the withdrawal's parameters at the end of the withdrawDeposit function. If for any reason reentrancy occurs, the value of the deposit will be always the same until it gets saved at the end allowing an attacker to withdraw more funds than the allowed one. As this is a function where funds are transferred out of the contract based on the named variables' values, consider changing memory to storage to keep the deposit updated in the contract storage at every property change and also adding a reentrancy guard to avoid any further violation. In the current implementation, the only calls are to SXUTToken and token which makes possible a reentrancy attack by those contracts. Update: The team fixed it on ced8378b1424d99b33c01f2a351902ac7bb3019d by adding a reentrancy guard plus changing memory to storage. Note that deposits[depositId] = deposit is no longer necessary.

  • L4 - line 458 - Difference fees computed - deposit.withdrawalFee is being update with fee but then if it is an early withdrawal, the withdrawalFee is being updated with fee - poolFee. This is creating a difference between what is stored vs what is transferred to the feeCollector.

Notes

  • N1 - line 240 - Hardcoded Token address - If the contract needs to be deployed in another chain and the SXUTToken has a different address in that chain, the contract code must be changed. Consider initializing SXUTToken in the contract constructor.

  • N2 - line 249 and 250 - Unused events - depositPartiallyWithdarwn and depositWithdarwn are defined but not used. Consider removing them from the code. Update: The team started using them on ced8378b1424d99b33c01f2a351902ac7bb3019d but depositEarlyWithdarwn is never going to be called because it has the same if logic as depositPartiallyWithdarwn

  • N3 - lines 254 and 255 - Obsolete initialization - Every uint is initialized with 0, it is the default value, when the contract is deployed. Update: The team fixed it on ced8378b1424d99b33c01f2a351902ac7bb3019d by removing the initializations.

  • N4 - lines 257 and 258 - The contract creator is being added as an admin duplicating code that is at addAdmin. Consider using a addAdmin internal version removing the check about the sender being a current admin to avoid code duplication and human mistakes by keeping only one source of truth. Update: The team fixed it on 0b1d477e7fec56db570ca41fefe6345176c534ef.

  • N5 - lines 264, 571, 580 - Obsolete variable - Storage variables can be returned directly instead of assign and return new variables. Update: The team fixed it on ced8378b1424d99b33c01f2a351902ac7bb3019d

  • N6 - lines 324, 333 - Obsolete and unused return variable: hodlAdminsList, isAdmin. Update: The team fixed it on ced8378b1424d99b33c01f2a351902ac7bb3019d

  • N7 - lines 338, 390, 722, 723, 747, 748 - TODO in the code. Update: The team fixed it on ced8378b1424d99b33c01f2a351902ac7bb3019d

  • N8 - lines 346, 434, 470, 471, 474, 475 - Commented code. Update: The team fixed it on ced8378b1424d99b33c01f2a351902ac7bb3019d but the comment in the line 346, now 212, not.

  • N9 - line 359 - Obsolete variable - getBonusEligibleSXUTBalance can just return bonusEligibleSXUTMinBalance without the need of assigning a new variable.

  • N10 - line 394 - Forced owner - The owner of the deposit is msg.sender. Users may need to set a different address as the owner/manager of it. Consider adding a beneficiary parameter who acts as the owner of the deposit.

  • N11 - line 402 - Period was already cast in line 388. Consider using it to reduce gas consumption.

  • N12 - line 408 - Possible overflow - Even as it is unlikely to happen, if a tokenWiseDepositCount reaches the maximum uint possible, will occur an overflow resetting its value starting from 0 again. Consider using SafeMath to prevent possible overflows. Update: The team fixed it on 0b1d477e7fec56db570ca41fefe6345176c534ef

  • N13 - line 443 - Obsolete variable assigment. amount has already amount value.

  • N14 - line 452, 458, 463, 464, 465, 479, 505, 506, 731, 735, 756, 760 - Missing the usage of SafeMath. Update: The team fixed it on 0b1d477e7fec56db570ca41fefe6345176c534ef but the line 452 (now 322), 463 (now 333), 464 (now 334), 479 (now 346), 505 (now 371), 506 (now 372), 731 (now 607), 735 (now 607), 756 (now 626), 760 (now 630) not. The contract is using Solidity 0.8. Therefore, the usage of SafeMath is not necessary.

  • N15 - lines 518, 529, 540, 551, and 562 - Obsolete is-admin check - msg.sender can be impersonated on eth_call's. Moreover, everything in the blockchain is public. Consider using private variables only when you are considering for example contract inheritance.

  • N16 - line 647 - Wrong dev notation - The method is to get the details of a withdrawal, not a deposit.

  • N17 - line 704 - typo: percenatge.

Final Thoughts

  • Tests: Consider adding tests for every function. Unit and integration tests help to find bugs and make the contract more solid.

Ignacio Mazzara - 07/2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment