Reentrancy vulnerability Staking.sol

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L674-L719

Vulnerability details

Impact

Possible Reentrancy attack function does not follow check effect interact pattern leaving it open to a possible reentrnacy attack re-entrancy attack can lead to a function being called again leading to the removal of more funds than are alloted/allowed by the external caller, Contracts should not make any changes to state variables, after interacting with external entities, as we cannot rely on the execution of any code coming after the interaction.

Proof of Concept

(direct link to code in question) https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L674-L719

personal automated test outcome

Reentrancy in Staking.unstake(uint256,bool) (src/contracts/Staking.sol#674-696): External calls:

• rebase() (src/contracts/Staking.sol#678)
  • IYieldy(YIELDY_TOKEN).rebase(epoch.distribute,epoch.number) (src/contracts/Staking.sol#704)
• _retrieveBalanceFromUser(_amount,msg.sender) (src/contracts/Staking.sol#680)
  • returndata = address(token).functionCall(data,SafeERC20: low-level call failed) (node_modules/@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol#93)
  • (success,returndata) = target.call{value: value}(data) (node_modules/@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol#137)
  • IERC20Upgradeable(YIELDY_TOKEN).safeTransferFrom(_user,address(this),amountLeft) (src/contracts/Staking.sol#559-563)
• claimWithdraw(msg.sender) (src/contracts/Staking.sol#685)
  • returndata = address(token).functionCall(data,SafeERC20: low-level call failed) (node_modules/@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol#93)
  • requestedWithdrawals = tokePoolContract.requestedWithdrawals(address(this)) (src/contracts/Staking.sol#300-301)
  • requestedWithdrawals = tokePoolContract.requestedWithdrawals(address(this)) (src/contracts/Staking.sol#281-282)
  • (success,returndata) = target.call{value: value}(data) (node_modules/@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol#137)
  • tokePoolContract.withdraw(requestedWithdrawals.amount) (src/contracts/Staking.sol#308)
  • IERC20Upgradeable(STAKING_TOKEN).safeTransfer(_recipient,info.amount) (src/contracts/Staking.sol#498-501)
  • IYieldy(YIELDY_TOKEN).burn(address(this),totalAmountIncludingRewards) (src/contracts/Staking.sol#503-506) External calls sending eth:
• _retrieveBalanceFromUser(_amount,msg.sender) (src/contracts/Staking.sol#680)
  • (success,returndata) = target.call{value: value}(data) (node_modules/@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol#137)
• claimWithdraw(msg.sender) (src/contracts/Staking.sol#685)
  • (success,returndata) = target.call{value: value}(data) (node_modules/@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol#137) State variables written after the call(s):
• coolDownInfo[msg.sender] = Claim(userCoolInfo.amount + _amount,userCoolInfo.credits + IYieldy(YIELDY_TOKEN).creditsForTokenBalance(_amount),epoch.number + coolDownPeriod) (src/contracts/Staking.sol#687-692)
• requestWithdrawalAmount += _amount (src/contracts/Staking.sol#694)

RequestWithdraw can be called multiple times possible ability to withdraw more funds than alloted/allowed

Tools Used

Sither

Recommended Mitigation Steps

ensure functions follow the correct check, effect, interact pattern and add additional checks in place where needed to stop potential reentrancy attacks however small the threat of maybe

[toshiSat]

73