feat: add extensive validation (#58)

src/Grants.sol

@@ -80,11 +80,7 @@ contract Grants {
         uint256 perPeriodAmount,
         address cancelAuthority
     ) external {
-        _mustBeNonZeroAddress(to);
-        _mustNotBeSelf(to);
-        _mustBeNonZero(period);
-        _mustBeNonZero(periodCount);
-        _mustBeEqualOrExceedMinAmount(perPeriodAmount);
+        validateVestingSchedule(to, period, periodCount, perPeriodAmount);
 
         token.safeTransferFrom(msg.sender, address(this), perPeriodAmount * periodCount);
 
@@ -100,6 +96,24 @@ contract Grants {
         emit VestingScheduleAdded(to, schedule);
     }
 
+    /**
+     * @notice Checks if the vesting schedule parameters are valid.
+     * @param to Recipient of the tokens under the vesting schedule.
+     * @param period Duration of each period in seconds.
+     * @param periodCount Number of periods in the vesting schedule.
+     * @param perPeriodAmount Amount of tokens to be released each period.
+     */
+    function validateVestingSchedule(address to, uint256 period, uint32 periodCount, uint256 perPeriodAmount)
+        public
+        view
+    {
+        _mustBeNonZeroAddress(to);
+        _mustNotBeSelf(to);
+        _mustBeNonZero(period);
+        _mustBeNonZero(periodCount);
+        _mustBeEqualOrExceedMinAmount(perPeriodAmount);
+    }
+
     /**
      * @notice Claims all vested tokens available for msg.sender up to the current block timestamp.
      * @param start Start page of the vesting schedules to claim from.

src/Rewards.sol

@@ -228,7 +228,7 @@ contract Rewards is AccessControl, EIP712 {
         _mustBeExpectedSequence(reward.recipient, reward.sequence);
         _mustBeFromAuthorizedOracle(digestReward(reward), signature);
 
-        _checkedUpdateQuota();
+        _checkedResetClaimed();
         _checkedUpdateClaimed(reward.amount);
 
         // Safe to increment the sequence after checking this is the expected number (no overflow for the age of universe even with 1000 reward claims per second)
@@ -251,7 +251,7 @@ contract Rewards is AccessControl, EIP712 {
 
         _mustBeFromAuthorizedOracle(digest, signature);
 
-        _checkedUpdateQuota();
+        _checkedResetClaimed();
 
         uint256 batchSum = _batchSum(batch);
         uint256 submitterRewardAmount = (batchSum * batchSubmitterRewardBasisPoints) / BASIS_POINTS_DIVISOR;
@@ -284,11 +284,10 @@ contract Rewards is AccessControl, EIP712 {
     }
 
     /**
-     * @dev Internal function to update the rewards quota if the current block timestamp is greater than or equal to the quota renewal timestamp.
      * @notice This function resets the rewards claimed to 0 and updates the quota renewal timestamp based on the reward period.
      * @notice The following operations are safe based on the constructor's requirements for longer than the age of the universe.
      */
-    function _checkedUpdateQuota() internal {
+    function _checkedResetClaimed() internal {
         if (block.timestamp >= quotaRenewalTimestamp) {
             claimed = 0;
 

src/bridge/BridgeBase.sol

@@ -56,15 +56,20 @@ abstract contract BridgeBase {
     /// @notice Error to indicate that the number of oracles exceeds the allowed maximum.
     error MaxOraclesExceeded();
 
+    /// @notice Error to indicate that the number of oracles is less than the minimum required.
+    error NotEnoughOracles();
+
+    /// @notice Error to indicate an invalid value for the minimum votes.
+    error InvalidZeroForMinVotes();
+
     /// @notice Initializes the contract with specified parameters.
     /// @param bridgeOracles Array of oracle accounts.
     /// @param token Contract address of the NODL token.
     /// @param minVotes Minimum required votes to consider a proposal valid.
     /// @param minDelay Blocks to wait before a passed proposal can be executed.
     constructor(address[] memory bridgeOracles, NODL token, uint8 minVotes, uint256 minDelay) {
-        require(bridgeOracles.length >= minVotes, "Not enough oracles");
-        require(minVotes > 0, "Votes must be more than zero");
-
+        _mustNotBeZeroMinVotes(minVotes);
+        _mustHaveEnoughOracles(bridgeOracles.length, minVotes);
         _mustNotExceedMaxOracles(bridgeOracles.length);
 
         for (uint256 i = 0; i < bridgeOracles.length; i++) {
@@ -171,6 +176,12 @@ abstract contract BridgeBase {
         }
     }
 
+    function _mustHaveEnoughOracles(uint256 length, uint256 minOracles) internal pure {
+        if (length < minOracles) {
+            revert NotEnoughOracles();
+        }
+    }
+
     function _mustBeAnOracle(address maybeOracle) internal view {
         if (!isOracle[maybeOracle]) {
             revert NotAnOracle(maybeOracle);
@@ -183,6 +194,12 @@ abstract contract BridgeBase {
         }
     }
 
+    function _mustNotBeZeroMinVotes(uint8 value) internal pure {
+        if (value == 0) {
+            revert InvalidZeroForMinVotes();
+        }
+    }
+
     function _mustNotHaveVotedYet(bytes32 proposal, address oracle) internal view {
         if (voted[oracle][proposal]) {
             revert AlreadyVoted(proposal, oracle);

src/bridge/GrantsMigration.sol

@@ -7,6 +7,9 @@ import {NODL} from "../NODL.sol";
 import {BridgeBase} from "./BridgeBase.sol";
 
 contract GrantsMigration is BridgeBase {
+    /// @notice Maximum number of vesting schedules allowed per proposal. The 100 limit is coming from the Eden parachain.
+    uint8 public constant MAX_SCHEDULES = 100;
+
     /// @dev Represents the vesting details of a proposal.
     struct Proposal {
         address target; // Address of the grant recipient.
@@ -29,6 +32,21 @@ contract GrantsMigration is BridgeBase {
     // Events
     event Granted(bytes32 indexed proposal, address indexed user, uint256 amount, uint256 numOfSchedules);
 
+    /**
+     * @notice Error to indicate that the schedule array is empty.
+     */
+    error EmptySchedules();
+
+    /**
+     * @notice Error to indicate that the schedule array is too large.
+     */
+    error TooManySchedules();
+
+    /**
+     * @notice Error to indicate that the schedules do not add up to the total amount.
+     */
+    error AmountMismatch();
+
     /**
      * @param bridgeOracles Array of addresses authorized to initiate and vote on proposals.
      * @param token Address of the NODL token used for grants.
@@ -125,6 +143,22 @@ contract GrantsMigration is BridgeBase {
         uint256 amount,
         Grants.VestingSchedule[] memory schedules
     ) internal {
+        if (schedules.length == 0) {
+            revert EmptySchedules();
+        }
+        if (schedules.length > MAX_SCHEDULES) {
+            revert TooManySchedules();
+        }
+        uint256 totalAmount = 0;
+        for (uint256 i = 0; i < schedules.length; i++) {
+            grants.validateVestingSchedule(
+                user, schedules[i].period, schedules[i].periodCount, schedules[i].perPeriodAmount
+            );
+            totalAmount += schedules[i].perPeriodAmount * schedules[i].periodCount;
+        }
+        if (totalAmount != amount) {
+            revert AmountMismatch();
+        }
         proposals[proposal] = Proposal({target: user, amount: amount, schedules: schedules});
         super._createVote(proposal, oracle, user, amount);
     }

src/bridge/MigrationNFT.sol

@@ -14,6 +14,10 @@ contract MigrationNFT is ERC721 {
     uint256[] public levels;
     string[] public levelToTokenURI;
 
+    error InvalidZeroHolders();
+
+    error InvalidZeroLevels();
+
     /**
      * @notice Mapping of token IDs to the levels they represent denominated from 1 (0 means token does not exists)
      */
@@ -49,20 +53,26 @@ contract MigrationNFT is ERC721 {
         uint256[] memory _levels,
         string[] memory _levelToTokenURI
     ) ERC721("OG ZK NODL", "OG_ZK_NODL") {
-        migration = _migration;
-        maxHolders = _maxHolders;
-        levels = _levels;
-        levelToTokenURI = _levelToTokenURI;
-
+        if (_maxHolders == 0) {
+            revert InvalidZeroHolders();
+        }
+        if (_levels.length == 0) {
+            revert InvalidZeroLevels();
+        }
         if (_levels.length != _levelToTokenURI.length) {
             revert UnequalLengths();
         }
 
-        for (uint256 i = 1; i < levels.length; i++) {
-            if (levels[i] <= levels[i - 1]) {
+        for (uint256 i = 1; i < _levels.length; i++) {
+            if (_levels[i] <= _levels[i - 1]) {
                 revert UnsortedLevelsList();
             }
         }
+
+        migration = _migration;
+        maxHolders = _maxHolders;
+        levels = _levels;
+        levelToTokenURI = _levelToTokenURI;
     }
 
     /**

test/bridge/GrantsMigration.t.sol

@@ -187,7 +187,7 @@ contract GrantsMigrationTest is Test {
     function test_executionFailsIfInsufficientVotes() public {
         bytes32 paraTxHash = keccak256(abi.encodePacked("tx6"));
         vm.prank(oracles[0]);
-        migration.bridge(paraTxHash, user, 100, schedules);
+        migration.bridge(paraTxHash, user, amount, schedules);
 
         vm.roll(block.number + delay + 1);
 
@@ -198,10 +198,10 @@ contract GrantsMigrationTest is Test {
     function test_executionFailsIfTooEarly() public {
         bytes32 paraTxHash = keccak256(abi.encodePacked("tx7"));
         vm.prank(oracles[0]);
-        migration.bridge(paraTxHash, user, 100, schedules);
+        migration.bridge(paraTxHash, user, amount, schedules);
 
         vm.prank(oracles[1]);
-        migration.bridge(paraTxHash, user, 100, schedules);
+        migration.bridge(paraTxHash, user, amount, schedules);
 
         vm.expectRevert(abi.encodeWithSelector(BridgeBase.NotYetWithdrawable.selector, paraTxHash));
         migration.grant(paraTxHash);
@@ -216,4 +216,46 @@ contract GrantsMigrationTest is Test {
         vm.expectRevert(abi.encodeWithSelector(BridgeBase.MaxOraclesExceeded.selector));
         new GrantsMigration(manyOracles, nodl, grants, uint8(manyOracles.length / 2 + 1), delay);
     }
+
+    function test_createProposalFailsWithEmptySchedules() public {
+        bytes32 paraTxHash = keccak256(abi.encodePacked("tx8"));
+        Grants.VestingSchedule[] memory emptySchedules;
+        vm.prank(oracles[0]);
+        vm.expectRevert(GrantsMigration.EmptySchedules.selector);
+        migration.bridge(paraTxHash, user, amount, emptySchedules);
+    }
+
+    function test_createProposalFailIfTooManySchedules() public {
+        bytes32 paraTxHash = keccak256(abi.encodePacked("tx9"));
+        Grants.VestingSchedule[] memory manySchedules = new Grants.VestingSchedule[](migration.MAX_SCHEDULES() + 1);
+        for (uint256 i = 0; i < manySchedules.length; i++) {
+            manySchedules[i] = schedules[0];
+        }
+        vm.prank(oracles[0]);
+        vm.expectRevert(GrantsMigration.TooManySchedules.selector);
+        migration.bridge(paraTxHash, user, amount, manySchedules);
+    }
+
+    function test_createProposalFailsIfAmountMismatch() public {
+        bytes32 paraTxHash = keccak256(abi.encodePacked("tx10"));
+        vm.prank(oracles[0]);
+        vm.expectRevert(GrantsMigration.AmountMismatch.selector);
+        migration.bridge(paraTxHash, user, amount + 1, schedules);
+    }
+
+    function test_createProposalFailsIfVestingScheduleInvalid() public {
+        bytes32 paraTxHash = keccak256(abi.encodePacked("tx11"));
+        Grants.VestingSchedule[] memory invalidSchedules = new Grants.VestingSchedule[](1);
+        invalidSchedules[0] = Grants.VestingSchedule({
+            start: block.timestamp + 1 days,
+            period: 1 days,
+            periodCount: 5,
+            perPeriodAmount: 0,
+            cancelAuthority: user
+        });
+        schedules[0].perPeriodAmount = 0;
+        vm.prank(oracles[0]);
+        vm.expectRevert(abi.encodeWithSelector(Grants.LowVestingAmount.selector));
+        migration.bridge(paraTxHash, user, amount, invalidSchedules);
+    }
 }

test/bridge/MigrationNFT.t.sol

@@ -61,6 +61,11 @@ contract MigrationNFTTest is Test {
         }
     }
 
+    function test_zeroHoldersWillRevert() public {
+        vm.expectRevert(MigrationNFT.InvalidZeroHolders.selector);
+        new MigrationNFT(migration, 0, levels, levelToTokenURI);
+    }
+
     function test_enforceSorting() public {
         uint256[] memory unsortedLevels = new uint256[](5);
         unsortedLevels[0] = 1000;