From 59eeba72b07a45b071713e6b96739a7094af14fe Mon Sep 17 00:00:00 2001 From: Liron Achdut Date: Sat, 28 Dec 2024 04:16:28 +0700 Subject: [PATCH] liron's `ButteredBread` fixes (#120) * add amount 0 checks (issue #114) * check transfer return value (issue #115) * move transfer to the end of _deposit (issue #116) also moved `_syncDelegation` just for consistency * fix natspec for balanceOfLP (issue #118) * _syncVotingWeight in deposit (issue #119) * add warning to modifyAllowList (issue #117) * custom errors * add comment about ButteredBread changes * revert move transferFrom * add reentrancy guard on deposit also added __ERC20Votes_init which does nothing but might change in future versions * add nonReentrant on withdraw --- src/ButteredBread.sol | 44 ++++++++++++++++++++----------- src/interfaces/IButteredBread.sol | 4 +++ 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/ButteredBread.sol b/src/ButteredBread.sol index ac46a9d..8af03f0 100644 --- a/src/ButteredBread.sol +++ b/src/ButteredBread.sol @@ -5,6 +5,8 @@ import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/a import {ERC20VotesUpgradeable} from "openzeppelin-contracts-upgradeable/contracts/token/ERC20/extensions/ERC20VotesUpgradeable.sol"; import {IERC20} from "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol"; +import {ReentrancyGuardUpgradeable} from + "openzeppelin-contracts-upgradeable/contracts/utils/ReentrancyGuardUpgradeable.sol"; import {IButteredBread} from "src/interfaces/IButteredBread.sol"; import {IERC20Votes} from "src/interfaces/IERC20Votes.sol"; @@ -17,7 +19,7 @@ import {IERC20Votes} from "src/interfaces/IERC20Votes.sol"; * @custom:coauthor @daopunk * @custom:coauthor @bagelface */ -contract ButteredBread is IButteredBread, ERC20VotesUpgradeable, OwnableUpgradeable { +contract ButteredBread is IButteredBread, ERC20VotesUpgradeable, OwnableUpgradeable, ReentrancyGuardUpgradeable { /// @notice Value used for calculating the precision of scaling factors uint256 public constant FIXED_POINT_PERCENT = 100; /// @notice `IERC20Votes` contract used for powering `ButteredBread` voting @@ -47,6 +49,8 @@ contract ButteredBread is IButteredBread, ERC20VotesUpgradeable, OwnableUpgradea __Ownable_init(msg.sender); __ERC20_init(_initData.name, _initData.symbol); + __ERC20Votes_init(); + __ReentrancyGuard_init(); for (uint256 i; i < _initData.liquidityPools.length; ++i) { scalingFactors[_initData.liquidityPools[i]] = _initData.scalingFactors[i]; @@ -70,26 +74,31 @@ contract ButteredBread is IButteredBread, ERC20VotesUpgradeable, OwnableUpgradea } /** - * @notice Deposit LP tokens + * @notice Deposit LP tokens and mint ButteredBread + * NOTE: Additional ButteredBread can be minted/burned due to scaling factor changes * @param _lp Liquidity Pool token * @param _amount Value of LP token */ - function deposit(address _lp, uint256 _amount) external onlyAllowed(_lp) { + function deposit(address _lp, uint256 _amount) external onlyAllowed(_lp) nonReentrant { + if (_amount == 0) revert AmountZero(); _deposit(msg.sender, _lp, _amount); } /** - * @notice Withdraw LP tokens + * @notice Withdraw LP tokens and burn ButteredBread + * NOTE: Additional ButteredBread can be minted/burned due to scaling factor changes * @param _lp Liquidity Pool token * @param _amount Value of LP token */ - function withdraw(address _lp, uint256 _amount) external onlyAllowed(_lp) { + function withdraw(address _lp, uint256 _amount) external onlyAllowed(_lp) nonReentrant { + if (_amount == 0) revert AmountZero(); _withdraw(msg.sender, _lp, _amount); } /** * @notice Allow or deny LP token * @dev Must set scaling factor before sanctioning LP token + * @dev WARNING: When adding a new LP token, ensure that it has 18 decimals * @param _lp Liquidity Pool token * @param _allowed Sanction status of LP token */ @@ -123,25 +132,27 @@ contract ButteredBread is IButteredBread, ERC20VotesUpgradeable, OwnableUpgradea revert NonDelegatable(); } - /// @notice Get the balance of a specific LP for a given account - /// @param _holder The address of the account to get the balance for - /// @param _lp The address of the LP to get the balance for - /// @return LPData memory The balance of the LP for the given account + /// @notice Get the balance and scaling factor of a specific LP for a given account + /// @param _holder The address of the account to get the data for + /// @param _lp The address of the LP to get the data for + /// @return LPData memory The balance and scaling factor of the LP for the given account function balanceOfLP(address _holder, address _lp) external view returns (LPData memory) { return _accountToLPData[_holder][_lp]; } /// @notice Deposit LP tokens and mint ButteredBread with corresponding LP scaling factor function _deposit(address _account, address _lp, uint256 _amount) internal { - IERC20(_lp).transferFrom(_account, address(this), _amount); - _accountToLPData[_account][_lp].balance += _amount; - - uint256 currentScalingFactor = scalingFactors[_lp]; - _accountToLPData[_account][_lp].scalingFactor = currentScalingFactor; + bool success = IERC20(_lp).transferFrom(_account, address(this), _amount); + if (!success) revert TransferFailed(); - _mint(_account, _amount * currentScalingFactor / FIXED_POINT_PERCENT); _syncDelegation(_account); + /// @dev ensure proper accounting in case of admin error in `modifyScalingFactor` where not all holders are updated + _syncVotingWeight(_account, _lp); + _accountToLPData[_account][_lp].balance += _amount; + + _mint(_account, _amount * scalingFactors[_lp] / FIXED_POINT_PERCENT); + emit ButterAdded(_account, _lp, _amount); } @@ -155,7 +166,8 @@ contract ButteredBread is IButteredBread, ERC20VotesUpgradeable, OwnableUpgradea _accountToLPData[_account][_lp].balance -= _amount; _burn(_account, _amount * scalingFactors[_lp] / FIXED_POINT_PERCENT); - IERC20(_lp).transfer(_account, _amount); + bool success = IERC20(_lp).transfer(_account, _amount); + if (!success) revert TransferFailed(); emit ButterRemoved(_account, _lp, _amount); } diff --git a/src/interfaces/IButteredBread.sol b/src/interfaces/IButteredBread.sol index cc43cdc..d4eb27d 100644 --- a/src/interfaces/IButteredBread.sol +++ b/src/interfaces/IButteredBread.sol @@ -17,6 +17,10 @@ interface IButteredBread { error NonTransferable(); /// @notice Occurs when a dependent variable is not set error UnsetVariable(); + /// @notice Occurs when a transfer fails + error TransferFailed(); + /// @notice Occurs when an amount is 0 + error AmountZero(); /// @notice The event emitted when an LP Token (Butter) has been added event ButterAdded(address _account, address _lp, uint256 _amount);