Merge pull request #57 from hokunet/avichalp/audit-feedback

fix: address audit feedback

Author: avichalp

.github/workflows/coverage-check.yaml

@@ -21,7 +21,7 @@ jobs:
       - name: Set up Node.js
         uses: actions/setup-node@v2
         with:
-          node-version: '18'
+          node-version: "18"
 
       - name: Install Dependencies
         run: pnpm install
@@ -43,9 +43,15 @@ jobs:
       - name: Install forge-coverage
         run: forge install
 
+        # Add build step before coverage
+      - name: Build contracts
+        run: |
+          forge build --extra-output-files abi --extra-output-files storageLayout
+
       - name: Run Coverage
         run: forge coverage --skip "*FFI.t.sol" --report lcov
-    # Check fails if coverage percentage falls under 80%
+
+      # Check fails if coverage percentage falls under 80%
       - name: Check Coverage Threshold
         run: |
           COVERAGE=$(forge coverage --skip "*FFI.t.sol"--report summary | grep 'Total coverage:' | awk '{print $3}' | cut -d'%' -f1)

foundry.toml

@@ -8,7 +8,7 @@ extra_output = ["storageLayout"]
 gas_reports = ["Hoku"]
 solc_version = "0.8.26"
 optimizer = true
-optimizer_runs = 200
+optimizer_runs = 1000000
 ffi = true
 ast = true
 via_ir = true

remappings.txt

@@ -6,4 +6,5 @@ forge-std/=lib/forge-std/src/
 @openzeppelin/foundry-upgrades/=lib/openzeppelin-foundry-upgrades/src/
 @axelar-network/interchain-token-service/=lib/interchain-token-service/
 @axelar-network/axelar-gmp-sdk-solidity/=lib/axelar-gmp-sdk-solidity/
-@prb/math/=lib/prb-math/src/
+@prb/math/=lib/prb-math/src/
+solidity-cborutils/=lib/filecoin-solidity/lib/solidity-cborutils/

src/token/Faucet.sol

@@ -20,14 +20,18 @@ error InvalidFunding(address from, uint256 value);
 /// @param value The amount of tokens funded
 event Funding(address indexed from, uint256 value);
 
+/// @dev Error thrown when a transfer fails
+/// @param recipient The address of the recipient
+error TransferFailed(address recipient);
+
 /// @title Faucet Contract
 /// @dev A simple faucet contract for distributing tokens
 contract Faucet is Ownable {
     /// @dev Mapping to store the next allowed request time for each key
     mapping(string => uint256) internal _nextRequestAt;
 
     /// @dev Amount of tokens to drip per request
-    uint256 internal _dripAmount = 18;
+    uint256 internal _dripAmount = 5 ether;
 
     /// @dev Initializes the Faucet contract
     /// @dev Sets the contract deployer as the initial owner
@@ -48,7 +52,7 @@ contract Faucet is Ownable {
     /// @dev Allows users to fund the faucet
     /// @dev Reverts if the funding amount is 0 or less
     function fund() external payable {
-        if (msg.value <= 0) {
+        if (msg.value == 0) {
             revert InvalidFunding(msg.sender, msg.value);
         }
 
@@ -60,20 +64,25 @@ contract Faucet is Ownable {
     /// @param recipient The address to receive the tokens
     /// @param keys Array of keys to identify the recipient used in the _nextRequestAt mapping
     function drip(address payable recipient, string[] calldata keys) external onlyOwner {
-        uint256 keysLength = keys.length;
         uint256 amount = _dripAmount;
-        for (uint256 i = 0; i < keysLength; i++) {
-            if (_nextRequestAt[keys[i]] > block.timestamp) {
-                revert TryLater();
-            }
-        }
+
         if (address(this).balance < amount) {
             revert FaucetEmpty();
         }
-        for (uint256 i = 0; i < keysLength; i++) {
+
+        uint256 keysLength = keys.length;
+        for (uint256 i; i < keysLength; ++i) {
+            if (_nextRequestAt[keys[i]] > block.timestamp) {
+                revert TryLater();
+            }
             _nextRequestAt[keys[i]] = block.timestamp + (12 hours);
         }
-        recipient.transfer(amount);
+
+        // Handle transfer result
+        (bool success,) = recipient.call{value: amount}("");
+        if (!success) {
+            revert TransferFailed(recipient);
+        }
     }
 
     /// @dev Sets the amount of tokens to distribute per request

src/token/Hoku.sol

@@ -30,8 +30,8 @@ contract Hoku is
     address internal _interchainTokenService;
     bytes32 internal _itsSalt;
 
-    bytes32 public ADMIN_ROLE; // solhint-disable-line var-name-mixedcase
-    bytes32 public MINTER_ROLE; // solhint-disable-line var-name-mixedcase
+    bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE"); // solhint-disable-line var-name-mixedcase
+    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); // solhint-disable-line var-name-mixedcase
 
     /// @custom:oz-upgrades-unsafe-allow constructor
     constructor() {
@@ -53,10 +53,6 @@ contract Hoku is
         _itsSalt = itsSalt;
         deployer = msg.sender;
 
-        // Initialize roles
-        ADMIN_ROLE = keccak256("ADMIN_ROLE");
-        MINTER_ROLE = keccak256("MINTER_ROLE");
-
         _grantRole(ADMIN_ROLE, msg.sender);
         _grantRole(MINTER_ROLE, msg.sender);
         _setRoleAdmin(MINTER_ROLE, ADMIN_ROLE);

src/token/ValidatorGater.sol

@@ -10,9 +10,10 @@ import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/contracts/
 import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/contracts/proxy/utils/UUPSUpgradeable.sol";
 
 /// The power range that an approved validator can have.
+/// @dev Using uint128 for min/max to pack them into a single storage slot
 struct PowerRange {
-    uint256 min;
-    uint256 max;
+    uint128 min;
+    uint128 max;
 }
 
 /// This is a simple implementation of `IValidatorGater`. It makes sure the exact power change
@@ -28,6 +29,10 @@ contract ValidatorGater is IValidatorGater, UUPSUpgradeable, OwnableUpgradeable
 
     event ActiveStateChange(bool active, address account);
 
+    error InvalidRouteLength();
+    error InvalidRouteAddress(address invalidAddress);
+    error ContractNotActive();
+
     function initialize() public initializer {
         __Ownable_init(msg.sender);
         _active = true;
@@ -47,12 +52,22 @@ contract ValidatorGater is IValidatorGater, UUPSUpgradeable, OwnableUpgradeable
 
     modifier whenActive() {
         if (!_active) {
-            return; // Skip execution if not active
+            revert ContractNotActive();
         }
-        _; // Continue with function execution if active
+        _;
     }
 
     function setSubnet(SubnetID calldata id) external onlyOwner whenActive {
+        if (id.route.length == 0) {
+            revert InvalidRouteLength();
+        }
+
+        for (uint256 i = 0; i < id.route.length; i++) {
+            if (id.route[i] == address(0)) {
+                revert InvalidRouteAddress(address(0));
+            }
+        }
+
         subnet = id;
     }
 
@@ -63,7 +78,7 @@ contract ValidatorGater is IValidatorGater, UUPSUpgradeable, OwnableUpgradeable
 
     /// Only owner can approve the validator join request
     function approve(address validator, uint256 minPower, uint256 maxPower) external onlyOwner whenActive {
-        allowed[validator] = PowerRange({min: minPower, max: maxPower});
+        allowed[validator] = PowerRange({min: uint128(minPower), max: uint128(maxPower)});
     }
 
     /// Revoke approved power range

src/token/ValidatorRewarder.sol

@@ -9,6 +9,8 @@ import {Hoku} from "./Hoku.sol";
 
 import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/contracts/access/OwnableUpgradeable.sol";
 import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/contracts/proxy/utils/UUPSUpgradeable.sol";
+
+import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 import {UD60x18, ud} from "@prb/math/UD60x18.sol";
 
 /// @title ValidatorRewarder
@@ -17,6 +19,7 @@ import {UD60x18, ud} from "@prb/math/UD60x18.sol";
 /// @dev The rewarder is called by the subnet actor when a validator claims rewards.
 contract ValidatorRewarder is IValidatorRewarder, UUPSUpgradeable, OwnableUpgradeable {
     using SubnetIDHelper for SubnetID;
+    using SafeERC20 for Hoku;
 
     // ========== STATE VARIABLES ==========
 
@@ -30,35 +33,44 @@ contract ValidatorRewarder is IValidatorRewarder, UUPSUpgradeable, OwnableUpgrad
     Hoku public token;
 
     /// @notice The latest checkpoint height that rewards can be claimed for
+    /// @dev Using uint64 to match Filecoin's epoch height type and save gas when interacting with the network
     uint64 public latestClaimedCheckpoint;
 
-    /// @notice The inflation rate for the subnet
-    /// @dev The rate is expressed as a decimal*1e18.
-    /// @dev For example 5% APY is 0.0000928276004952% yield per checkpoint period.
-    /// @dev This is expressed as 928_276_004_952 or 0.000000928276004952*1e18.
-    uint256 public inflationRate;
-
     /// @notice The bottomup checkpoint period for the subnet.
     /// @dev The checkpoint period is set when the subnet is created.
     uint256 public checkpointPeriod;
 
     /// @notice The supply of HOKU tokens at each checkpoint
-    mapping(uint64 => uint256) public checkpointToSupply;
+    mapping(uint64 checkpointHeight => uint256 totalSupply) public checkpointToSupply;
+
+    /// @notice The inflation rate for the subnet
+    /// @dev The rate is expressed as a decimal*1e18.
+    /// @dev For example 5% APY is 0.0000928276004952% yield per checkpoint period.
+    /// @dev This is expressed as 928_276_004_952 or 0.000000928276004952*1e18.
+    uint256 public constant INFLATION_RATE = 928_276_004_952;
 
     // ========== EVENTS & ERRORS ==========
 
     event ActiveStateChange(bool active, address account);
+    event SubnetUpdated(SubnetID subnet, uint256 checkpointPeriod);
+    event CheckpointClaimed(uint64 indexed checkpointHeight, address indexed validator, uint256 amount);
 
     error SubnetMismatch(SubnetID id);
     error InvalidClaimNotifier(address notifier);
     error InvalidCheckpointHeight(uint64 claimedCheckpointHeight);
     error InvalidCheckpointPeriod(uint256 period);
+    error InvalidTokenAddress(address token);
+    error InvalidValidatorAddress(address validator);
+    error ContractNotActive();
 
     // ========== INITIALIZER ==========
 
     /// @notice Initializes the rewarder
     /// @param hokuToken The address of the HOKU token contract
     function initialize(address hokuToken) public initializer {
+        if (hokuToken == address(0)) {
+            revert InvalidTokenAddress(hokuToken);
+        }
         __Ownable_init(msg.sender);
         __UUPSUpgradeable_init();
         _active = true;
@@ -75,16 +87,17 @@ contract ValidatorRewarder is IValidatorRewarder, UUPSUpgradeable, OwnableUpgrad
         }
         subnet = subnetId;
         checkpointPeriod = period;
+        emit SubnetUpdated(subnetId, period);
     }
 
     // ========== PUBLIC FUNCTIONS ==========
 
     /// @notice Modifier to ensure the contract is active
     modifier whenActive() {
         if (!_active) {
-            return; // Skip execution if not active
+            revert ContractNotActive();
         }
-        _; // Continue with function execution if active
+        _;
     }
 
     /// @notice Indicates whether the gate is active or not
@@ -100,13 +113,6 @@ contract ValidatorRewarder is IValidatorRewarder, UUPSUpgradeable, OwnableUpgrad
         emit ActiveStateChange(active, msg.sender);
     }
 
-    /// @notice Sets the inflation rate for the subnet.
-    /// @dev Only the owner can set the inflation rate, and only when the contract is active
-    /// @param rate The new inflation rate
-    function setInflationRate(uint256 rate) external onlyOwner whenActive {
-        inflationRate = rate;
-    }
-
     /// @notice Notifies the rewarder that a validator has claimed a reward.
     /// @dev Only the subnet actor can notify the rewarder, and only when the contract is active.
     /// @param id The subnet that the validator belongs to
@@ -117,6 +123,9 @@ contract ValidatorRewarder is IValidatorRewarder, UUPSUpgradeable, OwnableUpgrad
         uint64 claimedCheckpointHeight,
         Consensus.ValidatorData calldata data
     ) external override whenActive {
+        if (data.validator == address(0)) {
+            revert InvalidValidatorAddress(data.validator);
+        }
         // Check that the rewarder is responsible for the subnet that the validator is claiming rewards for
         if (keccak256(abi.encode(id)) != keccak256(abi.encode(subnet))) {
             revert SubnetMismatch(id);
@@ -143,25 +152,26 @@ contract ValidatorRewarder is IValidatorRewarder, UUPSUpgradeable, OwnableUpgrad
             // Get the current supply of HOKU tokens
             uint256 currentSupply = token.totalSupply();
 
-            // Set the supply for the checkpoint
+            // Set the supply for the checkpoint and update latest claimed checkpoint
             checkpointToSupply[claimedCheckpointHeight] = currentSupply;
-            // Calculate the inflation amount for __this__ checkpoint
+            latestClaimedCheckpoint = claimedCheckpointHeight;
+
+            // Calculate rewards
             uint256 supplyDelta = calculateInflationForCheckpoint(currentSupply);
-            // Calculate the validator's share of the inflation for __this__ checkpoint
             uint256 validatorShare = calculateValidatorShare(data.blocksCommitted, supplyDelta);
-            // Mint the supply delta minus current validator's share to the Rewarder
+
+            // Perform external interactions after state updates
             token.mint(address(this), supplyDelta - validatorShare);
-            // Mint the validator's share to the validator
             token.mint(data.validator, validatorShare);
-            // Update the latest claimable checkpoint.
-            latestClaimedCheckpoint = claimedCheckpointHeight;
+            emit CheckpointClaimed(claimedCheckpointHeight, data.validator, validatorShare);
         } else {
             // Calculate the supply delta for the checkpoint
             uint256 supplyDelta = calculateInflationForCheckpoint(supplyAtCheckpoint);
             // Calculate the validator's share of the supply delta
             uint256 validatorShare = calculateValidatorShare(data.blocksCommitted, supplyDelta);
             // Transfer the validator's share of the supply delta to the validator
-            token.transfer(data.validator, validatorShare);
+            token.safeTransfer(data.validator, validatorShare);
+            emit CheckpointClaimed(claimedCheckpointHeight, data.validator, validatorShare);
         }
     }
 
@@ -170,9 +180,9 @@ contract ValidatorRewarder is IValidatorRewarder, UUPSUpgradeable, OwnableUpgrad
     /// @notice The internal method to calculate the supply delta for a checkpoint
     /// @param supply The token supply at the checkpoint
     /// @return The supply delta, i.e. the amount of new tokens minted for the checkpoint
-    function calculateInflationForCheckpoint(uint256 supply) internal view returns (uint256) {
+    function calculateInflationForCheckpoint(uint256 supply) internal pure returns (uint256) {
         UD60x18 supplyFixed = ud(supply);
-        UD60x18 inflationRateFixed = ud(inflationRate);
+        UD60x18 inflationRateFixed = ud(INFLATION_RATE);
         UD60x18 result = supplyFixed.mul(inflationRateFixed);
         return result.unwrap();
     }

test/ValidatorGater.t.sol

@@ -112,8 +112,7 @@ contract ValidatorGaterTest is Test {
     }
 
     function testUnactiveGater() public {
-        address validator = address(4);
-        // Must be possible to desactivate the gater
+        // Must be possible to deactivate the gater
         vm.expectRevert();
         gater.setActive(false); // Not the owner
         assertEq(gater.isActive(), true);
@@ -122,15 +121,35 @@ contract ValidatorGaterTest is Test {
         gater.setActive(false);
         assertEq(gater.isActive(), false);
 
-        // Must not execute functions if inactive
+        // Test all functions with whenActive modifier
         vm.startPrank(owner);
+
+        // Test approve
+        vm.expectRevert(ValidatorGater.ContractNotActive.selector);
         gater.approve(validator1, 10, 10);
 
-        (uint256 min, uint256 max) = gater.allowed(validator);
-        assertEq(min, 0);
-        assertEq(max, 0);
+        // Test revoke
+        vm.expectRevert(ValidatorGater.ContractNotActive.selector);
+        gater.revoke(validator1);
+
+        // Test setSubnet
+        vm.expectRevert(ValidatorGater.ContractNotActive.selector);
+        gater.setSubnet(subnet);
+
+        // Test isAllow
+        vm.expectRevert(ValidatorGater.ContractNotActive.selector);
+        gater.isAllow(validator1, 100);
+
+        // Test interceptPowerDelta
+        vm.expectRevert(ValidatorGater.ContractNotActive.selector);
+        gater.interceptPowerDelta(subnet, validator1, 0, 100);
 
         vm.stopPrank();
+
+        // Verify storage state remains unchanged
+        (uint256 min, uint256 max) = gater.allowed(validator1);
+        assertEq(min, 0);
+        assertEq(max, 0);
     }
 
     function testSubnetManagerIntegration() public {