bug fixes and testing improvements

.gitignore

@@ -7,4 +7,5 @@ out/
 report/
 lcov.info
 notes.md
-.gas-snapshot
+.gas-snapshot
+/docs

Makefile

@@ -74,4 +74,20 @@ test_file_block_debug :; FOUNDRY_PROFILE=$(PROFILE) forge test $(FORK_URL) $(MAT
 test_file_debug_test :; FOUNDRY_PROFILE=$(PROFILE) forge test $(FORK_URL) $(MATCH_PATH) $(MATCH_TEST) -vvv
 
 # runs single test within file with added verbosity for failing test from a given block: "make test_file_block_debug_test FILE=Minter TEST=testUnwrap"
-test_file_block_debug_test :; FOUNDRY_PROFILE=$(PROFILE) forge test $(FORK_URL) $(MATCH_PATH) $(MATCH_TEST) $(FORK_BLOCK) -vvv
+test_file_block_debug_test :; FOUNDRY_PROFILE=$(PROFILE) forge test $(FORK_URL) $(MATCH_PATH) $(MATCH_TEST) $(FORK_BLOCK) -vvv
+
+# | File                       | % Lines            | % Statements       | % Branches       | % Funcs          |
+# |----------------------------|--------------------|--------------------|------------------|------------------|
+# | src/RewardsDistributor.sol | 90.00% (126/140)   | 87.44% (174/199)   | 69.70% (46/66)   | 80.00% (12/15)   |
+# | src/Bribe.sol              | 92.47% (135/146)   | 93.44% (171/183)   | 78.38% (58/74)   | 78.95% (15/19)   |
+# | src/VotingEscrow.sol       | 93.96% (467/497)   | 94.13% (577/613)   | 77.65% (205/264) | 90.00% (72/80)   |
+# | src/Voter.sol              | 98.86% (173/175)   | 99.01% (200/202)   | 83.96% (89/106)  | 96.67% (29/30)   |
+# | src/Minter.sol             | 100.00% (48/48)    | 100.00% (61/61)    | 88.46% (23/26)   | 100.00% (9/9)    |
+# | src/FluxToken.sol          | 97.06% (66/68)     | 97.40% (75/77)     | 90.32% (56/62)   | 100.00% (19/19)  |
+# | src/RevenueHandler.sol     | 100.00% (94/94)    | 100.00% (115/115)  | 93.18% (41/44)   | 100.00% (15/15)  |
+# | src/RewardPoolManager.sol  | 100.00% (47/47)    | 100.00% (55/55)    | 94.12% (32/34)   | 100.00% (13/13)  |
+# | src/AlchemixGovernor.sol   | 100.00% (16/16)    | 100.00% (17/17)    | 100.00% (12/12)  | 100.00% (6/6)    |
+# | src/BaseGauge.sol          | 100.00% (13/13)    | 100.00% (13/13)    | 100.00% (14/14)  | 80.00% (4/5)     |
+# | src/BribeFactory.sol       | 100.00% (1/1)      | 100.00% (2/2)      | 100.00% (0/0)    | 100.00% (1/1)    |
+# | src/GaugeFactory.sol       | 100.00% (2/2)      | 100.00% (4/4)      | 100.00% (0/0)    | 100.00% (2/2)    |
+# | src/PassthroughGauge.sol   | 100.00% (2/2)      | 100.00% (2/2)      | 100.00% (0/0)    | 100.00% (1/1)    |

src/AlchemixGovernor.sol

@@ -61,19 +61,31 @@ contract AlchemixGovernor is L2Governor, L2GovernorVotes, L2GovernorVotesQuorumF
         emit AdminUpdated(pendingAdmin);
     }
 
+    /**
+     * @dev Set the % of total supply required to create a proposal
+     * @param numerator The new numerator value for the quorum fraction
+     */
     function setProposalNumerator(uint256 numerator) external {
         require(msg.sender == admin, "not admin");
         require(numerator <= MAX_PROPOSAL_NUMERATOR, "numerator too high");
         proposalNumerator = numerator;
         emit ProposalNumberSet(numerator);
     }
 
+    /**
+     * @dev Set the value for the voting delay as defined in L2Governor.sol
+     * @param newDelay The new number of days voting will be delayed by
+     */
     function setVotingDelay(uint256 newDelay) external {
         require(msg.sender == admin, "not admin");
         votingDelay = newDelay;
         emit VotingDelaySet(votingDelay);
     }
 
+    /**
+     * @dev Set the value for the voting period as defined in L2Governor.sol
+     * @param newPeriod The new number of days voting will be open for
+     */
     function setVotingPeriod(uint256 newPeriod) external {
         require(msg.sender == admin, "not admin");
         votingPeriod = newPeriod;

src/BaseGauge.sol

@@ -6,23 +6,34 @@ import "src/interfaces/IBribe.sol";
 import "src/interfaces/IBaseGauge.sol";
 import "src/interfaces/IVoter.sol";
 import "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
+import "openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
 
 /**
  * @title  Base Gauge
  * @notice Implementation of functionality that various gauge types use or extend
  * @notice Gauges are used to incentivize pools, they emit or passthrough reward tokens
  */
 abstract contract BaseGauge is IBaseGauge {
-    uint256 internal constant DURATION = 2 weeks; // Rewards released over voting period
+    using SafeERC20 for IERC20;
+
+    /// @notice Rewards released over voting period
+    uint256 internal constant DURATION = 2 weeks;
     uint256 internal constant BRIBE_LAG = 1 days;
     uint256 internal constant MAX_REWARD_TOKENS = 16;
 
-    address public ve; // Ve token used for gauges
-    address public bribe; // Address of bribe contract
-    address public voter; // Address of voter contract
+    /// @notice veALCX token used for gauges
+    address public ve;
+    /// @notice Address of bribe contract
+    address public bribe;
+    /// @notice Address of voter contract
+    address public voter;
+    /// @notice Address of admin
     address public admin;
+    /// @notice Address of pending admin
     address public pendingAdmin;
+    /// @notice Address that receives the ALCX rewards
     address public receiver;
+    /// @notice Address of the reward token
     address public rewardToken;
 
     // Re-entrancy check
@@ -34,21 +45,6 @@ abstract contract BaseGauge is IBaseGauge {
         _unlocked = 1;
     }
 
-    /*
-        View functions
-    */
-
-    function getVotingStage(uint256 timestamp) external pure returns (VotingStage) {
-        uint256 modTime = timestamp % (1 weeks);
-        if (modTime < BRIBE_LAG) {
-            return VotingStage.BribesPhase;
-        } else if (modTime >= BRIBE_LAG && modTime < (BRIBE_LAG + DURATION)) {
-            return VotingStage.VotesPhase;
-        } else {
-            return VotingStage.RewardsPhase;
-        }
-    }
-
     /*
         External functions
     */
@@ -63,6 +59,7 @@ abstract contract BaseGauge is IBaseGauge {
         admin = pendingAdmin;
     }
 
+    /// @inheritdoc IBaseGauge
     function updateReceiver(address _receiver) external {
         require(msg.sender == admin, "not admin");
         require(_receiver != address(0), "cannot be zero address");
@@ -74,7 +71,7 @@ abstract contract BaseGauge is IBaseGauge {
     function notifyRewardAmount(uint256 _amount) external lock {
         require(msg.sender == voter, "not voter");
         require(_amount > 0, "zero amount");
-        _safeTransferFrom(rewardToken, msg.sender, address(this), _amount);
+        IERC20(rewardToken).safeTransferFrom(msg.sender, address(this), _amount);
 
         emit NotifyReward(msg.sender, rewardToken, _amount);
 
@@ -85,20 +82,6 @@ abstract contract BaseGauge is IBaseGauge {
         Internal functions
     */
 
-    function _safeTransfer(address token, address to, uint256 value) internal {
-        require(token.code.length > 0);
-        (bool success, bytes memory data) = token.call(abi.encodeWithSelector(IERC20.transfer.selector, to, value));
-        require(success && (data.length == 0 || abi.decode(data, (bool))));
-    }
-
-    function _safeTransferFrom(address token, address from, address to, uint256 value) internal {
-        require(token.code.length > 0);
-        (bool success, bytes memory data) = token.call(
-            abi.encodeWithSelector(IERC20.transferFrom.selector, from, to, value)
-        );
-        require(success && (data.length == 0 || abi.decode(data, (bool))));
-    }
-
     /**
      * @notice Override function to implement passthrough logic
      * @param _amount Amount of rewards

src/Bribe.sol

@@ -1,31 +1,44 @@
 // SPDX-License-Identifier: GPL-3
 pragma solidity ^0.8.15;
-
+import "lib/forge-std/src/console2.sol";
 import "src/interfaces/IBribe.sol";
 import "src/interfaces/IBaseGauge.sol";
 import "src/interfaces/IVoter.sol";
 import "src/interfaces/IVotingEscrow.sol";
 import "src/libraries/Math.sol";
 import "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
+import "openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
 
 /**
  * @title  Bribe
  * @notice Implementation of bribe contract to be used with gauges
  */
 contract Bribe is IBribe {
-    uint256 internal constant DURATION = 2 weeks; // Rewards released over voting period
+    using SafeERC20 for IERC20;
+
+    /// @notice Rewards released over voting period
+    uint256 internal constant DURATION = 2 weeks;
+    /// @notice Duration of time when bribes are accepted
     uint256 internal constant BRIBE_LAG = 1 days;
+    /// @notice Maximum number of reward tokens a gauge can accept
     uint256 internal constant MAX_REWARD_TOKENS = 16;
 
     /// @notice The number of checkpoints
     uint256 public supplyNumCheckpoints;
+    /// @notice Number of voting period checkpoints
     uint256 public votingNumCheckpoints;
+    /// @notice Total votes allocated to the gauge
     uint256 public totalSupply;
+    /// @notice Total current votes in a voting period (this is reset each period)
     uint256 public totalVoting;
 
+    /// @notice veALCX contract address
     address public immutable veALCX;
+    /// @notice Voter contract address
     address public immutable voter;
-    address public gauge; // Address of the gauge that the bribes are for
+    /// @notice Address of the gauge that the bribes are for
+    address public gauge;
+    /// @notice List of reward tokens
     address[] public rewards;
 
     /// @notice A record of balance checkpoints for each account, by index
@@ -34,11 +47,17 @@ contract Bribe is IBribe {
     mapping(uint256 => uint256) public numCheckpoints;
     /// @notice A record of balance checkpoints for each token, by index
     mapping(uint256 => SupplyCheckpoint) public supplyCheckpoints;
+    /// @notice A record of balance checkpoints for each voting period
     mapping(uint256 => VotingCheckpoint) public votingCheckpoints;
+    /// @notice A record of reward tokens that are accepted
     mapping(address => bool) public isReward;
+    /// @notice A record of token rewards per epoch for each reward token
     mapping(address => mapping(uint256 => uint256)) public tokenRewardsPerEpoch;
+    /// @notice Current votes allocated of a veALCX voter
     mapping(uint256 => uint256) public balanceOf;
+    /// @notice The end of the current voting period
     mapping(address => uint256) public periodFinish;
+    /// @notice The last time rewards were claimed
     mapping(address => mapping(uint256 => uint256)) public lastEarn;
 
     // Re-entrancy check
@@ -74,6 +93,7 @@ contract Bribe is IBribe {
 
     /// @inheritdoc IBribe
     function lastTimeRewardApplicable(address token) public view returns (uint256) {
+        // Return the current period if it's still active, otherwise return the next period
         return Math.min(block.timestamp, periodFinish[token]);
     }
 
@@ -90,7 +110,7 @@ contract Bribe is IBribe {
 
     /// @inheritdoc IBribe
     function notifyRewardAmount(address token, uint256 amount) external lock {
-        require(amount > 0);
+        require(amount > 0, "reward amount must be greater than 0");
 
         // If the token has been whitelisted by the voter contract, add it to the rewards list
         require(IVoter(voter).isWhitelisted(token), "bribe tokens must be whitelisted");
@@ -100,7 +120,8 @@ contract Bribe is IBribe {
         uint256 adjustedTstamp = getEpochStart(block.timestamp);
         uint256 epochRewards = tokenRewardsPerEpoch[token][adjustedTstamp];
 
-        _safeTransferFrom(token, msg.sender, address(this), amount);
+        IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
+
         tokenRewardsPerEpoch[token][adjustedTstamp] = epochRewards + amount;
         periodFinish[token] = adjustedTstamp + DURATION;
 
@@ -109,37 +130,26 @@ contract Bribe is IBribe {
 
     /// @inheritdoc IBribe
     function addRewardToken(address token) external {
-        require(msg.sender == gauge);
+        require(msg.sender == gauge, "not being set by a gauge");
         _addRewardToken(token);
     }
 
     /// @inheritdoc IBribe
     function swapOutRewardToken(uint256 oldTokenIndex, address oldToken, address newToken) external {
-        require(msg.sender == voter);
-        require(IVoter(voter).isWhitelisted(newToken), "bribe tokens must be whitelisted");
-        require(rewards[oldTokenIndex] == oldToken);
-        require(newToken != address(0));
+        require(msg.sender == voter, "Only voter can execute");
+        require(IVoter(voter).isWhitelisted(newToken), "New token must be whitelisted");
+        require(rewards[oldTokenIndex] == oldToken, "Old token mismatch");
 
-        isReward[oldToken] = false;
-        isReward[newToken] = true;
-
-        // Check if the newToken already exists in the rewards list
+        // Check that the newToken does not already exist in the rewards array
         for (uint256 i = 0; i < rewards.length; i++) {
-            if (rewards[i] == newToken) {
-                // If it exists, swap out the old token
-                rewards[oldTokenIndex] = rewards[i];
-
-                // Then remove the duplicate
-                rewards[i] = rewards[rewards.length - 1];
-                rewards.pop();
-                break;
-            }
+            require(rewards[i] != newToken, "New token already exists");
         }
 
-        // If the old token wasn't updated, swap it out
-        if (rewards[oldTokenIndex] != newToken) {
-            rewards[oldTokenIndex] = newToken;
-        }
+        isReward[oldToken] = false;
+        isReward[newToken] = true;
+
+        // Since we've now ensured the new token doesn't exist, we can safely update
+        rewards[oldTokenIndex] = newToken;
 
         emit RewardTokenSwapped(oldToken, newToken);
     }
@@ -175,6 +185,7 @@ contract Bribe is IBribe {
         return lower;
     }
 
+    /// @inheritdoc IBribe
     function getPriorVotingIndex(uint256 timestamp) public view returns (uint256) {
         uint256 nCheckpoints = votingNumCheckpoints;
         if (nCheckpoints == 0) {
@@ -185,7 +196,6 @@ contract Bribe is IBribe {
         if (votingCheckpoints[nCheckpoints - 1].timestamp < timestamp) {
             return (nCheckpoints - 1);
         }
-
         // Check implicit zero balance
         if (votingCheckpoints[0].timestamp > timestamp) {
             return 0;
@@ -207,6 +217,7 @@ contract Bribe is IBribe {
         return lower;
     }
 
+    /// @inheritdoc IBribe
     function earned(address token, uint256 tokenId) public view returns (uint256) {
         if (numCheckpoints[tokenId] == 0) {
             return 0;
@@ -281,14 +292,14 @@ contract Bribe is IBribe {
             lastEarn[tokens[i]][tokenId] = block.timestamp;
 
             _writeCheckpoint(tokenId, balanceOf[tokenId]);
-            _writeSupplyCheckpoint();
 
-            _safeTransfer(tokens[i], _owner, _reward);
+            IERC20(tokens[i]).safeTransfer(_owner, _reward);
 
             emit ClaimRewards(_owner, tokens[i], _reward);
         }
     }
 
+    /// @inheritdoc IBribe
     function deposit(uint256 amount, uint256 tokenId) external {
         require(msg.sender == voter);
 
@@ -304,6 +315,7 @@ contract Bribe is IBribe {
         emit Deposit(msg.sender, tokenId, amount);
     }
 
+    /// @inheritdoc IBribe
     function withdraw(uint256 amount, uint256 tokenId) external {
         require(msg.sender == voter);
 
@@ -374,18 +386,4 @@ contract Bribe is IBribe {
     function _bribeStart(uint256 timestamp) internal pure returns (uint256) {
         return timestamp - (timestamp % (DURATION));
     }
-
-    function _safeTransfer(address token, address to, uint256 value) internal {
-        require(token.code.length > 0);
-        (bool success, bytes memory data) = token.call(abi.encodeWithSelector(IERC20.transfer.selector, to, value));
-        require(success && (data.length == 0 || abi.decode(data, (bool))));
-    }
-
-    function _safeTransferFrom(address token, address from, address to, uint256 value) internal {
-        require(token.code.length > 0);
-        (bool success, bytes memory data) = token.call(
-            abi.encodeWithSelector(IERC20.transferFrom.selector, from, to, value)
-        );
-        require(success && (data.length == 0 || abi.decode(data, (bool))));
-    }
 }

src/CurveEthPoolAdapter.sol

@@ -23,6 +23,7 @@ contract CurveEthPoolAdapter is IPoolAdapter {
         }
     }
 
+    /// @inheritdoc IPoolAdapter
     function getDy(
         address inputToken,
         address outputToken,
@@ -31,6 +32,7 @@ contract CurveEthPoolAdapter is IPoolAdapter {
         return ICurveStableSwap(pool).get_dy(tokenIds[inputToken], tokenIds[outputToken], inputAmount);
     }
 
+    /// @inheritdoc IPoolAdapter
     function melt(
         address inputToken,
         address outputToken,