# NM-0148 WORLDCOIN - INTERNAL NOTES

**Repo** https://github.com/worldcoin/worldcoin-grants-contracts/tree/9d22282f119f134024577031ce7fc46fd2cccb48

**Commit**: 9d22282f119f134024577031ce7fc46fd2cccb48

---

### [High] Possible double signaling in claim function

**File(s)**: [`RecurringGrantDropLegacy.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/RecurringGrantDropLegacy.sol#L251C3-L251C3), [`RecurringGrantDrop.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/RecurringGrantDrop.sol#L177)

**Description**: When `grantId` is in the interval `[13,20)`, users can claim either at the previous `RecurringGrantDrop` contract or the `RecurringGrantDropLegacy` contract.

These two contracts manage separate instances of the `nullifierHashes` mapping. The `RecurringGrantDropLegacy` contract verifies the previously stored value in `RecurringGrantDrop` before executing any claim. However, the other way around is not handled, opening the possibility of a user making a duplicate claim using the same `nullifierHash`. To illustrate this vulnerability, we provide a diagram showing the attack flow and detail the steps involved:

`<insert diagram>`

Consider a user attempting to claim `grantId` with `nullifierHash` 5. Initially, `nullifierHashes[5]` is set to `false` in both contracts.

- (1) User initiates a call to `RecurringGrantDropLegacy.claim(...)` with *nullifierHash == 5*.
- (2,3) Check for `!nullifierHashes[5]` is performed on both contracts.
- (4,5) `checkValidity(...)` is called on the grant contract to verify that the `grantId` is within the accepted range. The contract sets `nullifierHashes[5]` to `true` and transfers tokens to the user.
- (7) User initiates another claim call to `RecurringGrantDrop.claim(...)` with *nullifierHash = 5*.
- (8,9) The contract checks the `allowedCallers` and `nullifierHashes[5]` mappings. The mapping in this contract is not updated from the previous call, resulting in the check successfully passing.
- (10,11) `checkValidity(...)` is called on the grant contract,`nullifierHashes[5]` is set to `true` in the `RecurringGrantDrop` contract, and tokens are transferred to the user.

In this scenario, the user successfully claimed the tokens using the same `nullifierHash` twice.

**Recommendation(s)**: Managing two contracts with separate instances of the same storage data introduces complexity and increases the likelihood of errors and vulnerabilities. To mitigate this, it is advised to restrict claims within the `[13,20)` `grantId` range to be handled exclusively by the `RecurringGrantDrop` contract. This eliminates the possibility of duplicate claims and simplifies the flow.

**Status**: Fixed

**Update from the client**:
Deployment procedure was now changed to the following:
- Existing contract gets updated to use `LaunchGrantLegacy` (allowing claiming only until grantId 20)
- Claiming logic got removed from `RecurringGrantDropLegacy` (only reservation redemption)
- also has `LaunchGrantLegacy` set
- `RecurringGrantDrop` is deployed with `WLDGrant` (allowing both redemption of reservations and normal claiming starting grantId 21)

Also updated in the previously flawed [PR comment](https://github.com/worldcoin/worldcoin-grants-contracts/pull/7#issuecomment-1758596948).

---

### [Low] Centralization risk for intended behavior

**File(s)**: [`RecurringGrantDropLegacy.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/RecurringGrantDropLegacy.sol#L321C1-L321C1),[`RecurringGrantDrop.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/RecurringGrantDrop.sol#L302), [`WLDGrant.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/WLDGrant.sol#L27), [`LaunchGrantLegacy.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/LaunchGrantLegacy.sol#L33)

**Description**: The intended behavior of contracts differs based on specific `grantId` intervals. Users are expected to claim or redeem grants at either the previous `RecurringGrantDrop` contract or `RecurringGrantDropLegacy` contract for grantIds `[13,20)`, and at the new `RecurringGrantDrop` contract for grantIds `[20,∞)`.

A centralization risk emerges because the logic handling `grantId` intervals is implemented within the Grant contracts. Both `RecurringGrantDrop` and `RecurringGrantDropLegacy` contracts point to a specific Grant contract, and the owner can update its address through the function `setGrant(...)`. This gives the owner the possibility to disrupt the expected behavior by setting a wrong grant address.

**Recommendation(s)**: Consider revisiting the centralization risk within the protocol and whether it is acceptable for the owner to have this level of control over the intended behavior. If necessary, design changes should be made to the solution in order to ensure a less error-prone approach.

**Status**: Acknowledged

**Update from the client**: Acknowledged. Limitation of current design. Ownership will be handed over to a governance contract in the future.

---
### [Info] `allowedCallers` check can be bypassed for claiming process
**File(s)**: [`RecurringGrantDropLegacy.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/RecurringGrantDropLegacy.sol#L161), [`RecurringGrantDrop.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/d228cf5d18db3e754a34b43fdaa2a298f508fe39/src/RecurringGrantDrop.sol#L153C15-L153C28)

**Description**: Between `grantId` values 13 and 19, users can claim using either the previous `RecurringGrantDrop` contract or the newly deployed `RecurringGrantDropLegacy` contract. Both paths are expected to operate identically. However, this is not the case, as the `RecurringGrantDrop` contract enforces a caller validation check before allowing a claim. This check is absent in the `RecurringGrantDropLegacy` contract. Consequently, users can bypass this check by exclusively calling the claim function on the latter contract.

function claim(uint256 grantId, address receiver, uint256 root, uint256 nullifierHash, uint256[8] calldata proof) external {
    // @audit check is removed in the `RecurringGrantDropLegacy` contract. Users can bypass this check by calling `claim(...)` on the later contract instead. 
    if (!allowedCallers[msg.sender]) revert Unauthorized();
    ...
}

**Recommendation(s)**: Given that the `allowedCallers` check will be deprecated after `grantId` 20, it is important to verify if this is also the case for lower values when both `RecurringGrantDrop` and `RecurringGrantDropLegacy` contracts are operating. If not, we recommend updating the contract logic to ensure identical claiming behavior across the two contracts. This can be achieved by either integrating the `allowedCallers` in the `RecurringGrantDropLegacy` contract or restricting claims to be done exclusively on the previous `RecurringGrantDrop` contract.

**Status**: Acknowledged

**Update from the client**: `allowedCallers` is not relied on in production since the `MultiCall3` contract is also allowed caller. This check can be savely ignored.

---
### [Info] Unnecessary external call for the current timestamp

**File(s)**: [`RecurringGrantDropLegacy.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/RecurringGrantDropLegacy.sol#L228)

**Description**: In the function `checkClaimReserved(...)`, a timestamp is provided as input and used to fetch the corresponding `grantId`. This `grandtId` is necessary to check the reservation validity through a call to the function `checkReservationValidity(...)`.

If the provided `timestamp` indicates a future time, the function reverts. However, if the timestamp corresponds to the current block timestamp (`block.timestamp`), the `grantId` will represent the `currentGrantId` in both `LaunchGrantLegacy` and `WLDGrant` contracts. This leads to the `checkReservationValidity(...)` call always reverting.

function checkClaimReserved(
    uint256 timestamp,
    address receiver,
    uint256 root,
    uint256 nullifierHash,
    uint256[8] calldata proof,
    bytes calldata signature
) public {
    uint256 grantId = grant.calculateId(timestamp);

    if (receiver == address(0)) revert InvalidReceiver();
    if (timestamp > block.timestamp) revert InvalidTimestamp();
    ...
    
    // @audit call to `checkReservationValidity` always reverts with the current timestamp
    grant.checkReservationValidity(timestamp);
    ...
}

**Recommendation(s)**: Consider updating the timestamp check to avoid an unnecessary external call. Below is the code with the suggested change:

if (timestamp >= block.timestamp) revert InvalidTimestamp();

**Status**: Fixed

**Update from the client**: [Recommendation implemented.](https://github.com/worldcoin/worldcoin-grants-contracts/pull/7)

---

### [Info] Unnecessary external calls could be replaced by using internal calls

**File(s)**: [`LaunchGrantLegacy.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/LaunchGrantLegacy.sol), [`WLDGrant.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/WLDGrant.sol), [`RecurringGrantDropLegacy.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/RecurringGrantDropLegacy.sol#L230)

**Description**: When `this.functionName(...)` is used, the contract makes an external call to itself, causing the sender to become `address(this)`. This is useful when the target function's logic depends on the sender's address. However, in the context of `RecurringGrantDropLegacy`, `LaunchGrantLegacy`, and `WLDGrant` contracts, these are unnecessary. They can be replaced with normal internal calls to save gas while maintaining the same result. For instance, `this.calculateId(...)` can be changed to `calculateId(...)`.

function checkReservationValidity(uint256 timestamp) external view override {
    // @audit `this.calculateId()` is unnecessary and wasting gas
    uint256 grantId = this.calculateId(timestamp);

    // No future grants can be claimed.
    if (grantId >= this.getCurrentId()) revert InvalidGrant();

    // Only grants 20 and above can be reserved.
    if (grantId < 20) revert InvalidGrant();

    // Reservations are only valid for 12 months.
    if (block.timestamp > timestamp + 52 weeks) revert InvalidGrant();
}

**Recommendation(s)**: Consider using internal calls instead to save gas.

**Status**: Acknowledged

**Update from the client**: External calls are kept because of the grant interface.

---
### [Best Practices] External call before input validations

**File(s)**: [`RecurringGrantDropLegacy.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/RecurringGrantDropLegacy.sol#L228)

**Description**: The `checkClaimReserved(...)` function calls `grant.calculateId(...)` function to retrieve the `grantId`. This call is made before performing input checks, resulting in unneeded gas consumption if checks will revert.

  function checkClaimReserved(
    uint256 timestamp,
    address receiver,
    uint256 root,
    uint256 nullifierHash,
    uint256[8] calldata proof,
    bytes calldata signature
  ) public {
    uint256 grantId = grant.calculateId(timestamp);
    // @audit verifications can be done before the external call, to avoid unnecessary gas consumption
    if (receiver == address(0)) revert InvalidReceiver();
    if (timestamp > block.timestamp) revert InvalidTimestamp();
    ...
  }

**Recommendation(s)**: Consider reordering code to perform input checks before retrieving the `grantId`.

**Status**: Unresolved

**Update from the client**: [Recommendation implemented.](https://github.com/worldcoin/worldcoin-grants-contracts/pull/7)

**Update from Nethermind**: The suggested optimization in [`checkClaimReserved(...)`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/reservations/src/RecurringGrantDrop.sol#L219) function was not implemented.

---

### [Best Practices] Unused imports

**File(s)**: [`RecurringGrantDropLegacy.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/RecurringGrantDropLegacy.sol#L6), [`RecurringGrantDrop.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/RecurringGrantDrop.sol#L6)

**Description**: The following imports are not being used in the `RecurringGrantDrop` and `RecurringGrantDropLegacy` contracts.

// @audit imports are not used by the contract
import { SafeTransferLib } from 'solmate/utils/SafeTransferLib.sol';
...
import { IWorldID } from 'world-id-contracts/interfaces/IWorldID.sol';

**Recommendation(s)**: Consider removing unused imports from both contracts.

**Status**: Fixed

**Update from the client**: [Recommendation implemented.](https://github.com/worldcoin/worldcoin-grants-contracts/pull/7)

---

### [Best Practices] Wrong or missing NatSpec documentation

**File(s)**: [`RecurringGrantDropLegacy.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/RecurringGrantDropLegacy.sol#L15)

**Description**: The following issues have been identified in the `RecurringGrantDropLegacy` contract's NatSpec comments:

- Wrong title for contract:

        /// @title RecurringGrantDrop 
        // @audit wrong contract title 
        /// @author Worldcoin
        contract RecurringGrantDropLegacy is Ownable2Step {

- Absence of NatSpec description for `signature` input of `claimReserved(...)` function.
- Outdated `@dev` note referring to the removed `hashToField` function in both `claim(...)` and `claimReserved(...)` functions.

    /// @dev hashToField function docs are in lib/world-id-contracts/src/libraries/ByteHasher.sol
    function claim(

    /// @dev hashToField function docs are in lib/world-id-contracts/src/libraries/ByteHasher.sol
    function claimReserved(

**Recommendation(s)**: Consider implementing the documentation updates and fixes listed above.

**Status**: Fixed

**Update from the client**: [Recommendation implemented.](https://github.com/worldcoin/worldcoin-grants-contracts/pull/7)

**Update from Nethermind**: We confirm that the previous documentation issues were fixed. However, we noticed a few comments that needed to be corrected in the new commits. Consider fixing them.
- Comment using the previous `grantId` interval [here](https://github.com/worldcoin/worldcoin-grants-contracts/pull/7/commits/4595b452371bcae0a568b8ba32554a726466f4f3#diff-4a3e6f92839[…]986934884295b7R36).
- Date in [this comment](https://github.com/worldcoin/worldcoin-grants-contracts/pull/7/commits/4595b452371bcae0a568b8ba32554a726466f4f3#diff-5f3c6e6401a[…]29ed9b0cf67ddfR25) refers to the old timestamp.

---

### [Best Practices] Redundant call to `grant.calculateId(...)` in the function `claimReserved(...)`

**File(s)**: [`RecurringGrantDrop.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/RecurringGrantDrop.sol#L201), [`RecurringGrantDropLegacy.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/RecurringGrantDropLegacy.sol#L205)

**Description**: The function `claimReserved(...)` initially calls the grant contract to calculate `grantId`. It then uses the function `checkClaimReserved(...)` to verify the claim's validity. This function, however, performs the same call to the grant contract to calculate the `grantId` again, which is not efficient.

function claimReserved(uint256 timestamp, address receiver, uint256 root, uint256 nullifierHash, uint256[8] calldata proof, bytes calldata signature)
    external
{
    uint256 grantId = grant.calculateId(timestamp); // @audit First call to calculate ID
    checkClaimReserved(timestamp, receiver, root, nullifierHash, proof, signature);
    ...
}

function checkClaimReserved(uint256 timestamp, address receiver, uint256 root, uint256 nullifierHash, uint256[8] calldata proof, bytes calldata signature)
    public
{
    uint256 grantId = grant.calculateId(timestamp); // @audit Second call to calculate ID
    ...
}

**Recommendation(s)**: Consider passing `grantId` to the function `checkClaimReserved(...)` or have the function `checkClaimReserved(...)` return the value of `grantId` to avoid unnecessary external calls. Another possible solution is to introduce an internal function `_checkClaimReserved(...)` to avoid changing the interfaces of the two existing external functions.

**Status**: Acknowledged

**Update from the client**: Not implemented.

---

### [Best Practices] Redundant check for `grantId` less than 13 in the function `checkReservationValidity(...)`

**File(s)**: [`LaunchGrantLegacy.sol`](https://github.com/worldcoin/worldcoin-grants-contracts/blob/9d22282f119f134024577031ce7fc46fd2cccb48/src/LaunchGrantLegacy.sol#L43)

**Description**: The function `checkReservationValidity(...)` contains a redundant check for `grantId < 13`, which is unnecessary due to the constraints set by the function `calculateId(...)`. The function `calculateId(...)` ensures that `grantId` is always greater than or equal to 13 or triggers a revert.

// @audit Return value at least 13
function calculateId(uint256 timestamp) external pure returns (uint256) {
    if (timestamp < launchDayTimestampInSeconds) revert InvalidGrant();

    uint weeksSinceLaunch = (timestamp - launchDayTimestampInSeconds) / 1 weeks;
    // Monday, 24 July 2023 07:00:00 until Monday, 07 August 2023 06:59:59 (2 weeks)
    if (weeksSinceLaunch < 2) return 13;
    // Monday, 07 August 2023 03:00:00 until Monday, 14 August 2023 02:59:59 (1 week)
    if (weeksSinceLaunch < 3) return 14;
    return 15 + (weeksSinceLaunch - 3) / 2;
}

function checkReservationValidity(uint256 timestamp) external view override {
    uint256 grantId = this.calculateId(timestamp);
    ...
    // @audit Check for grantId < 13 is unnecessary
    if (grantId < 13 || grantId >= 20) revert InvalidGrant(); 
    ...
}

**Recommendation(s)**: Consider removing the redundant check in the function `checkReservationValidity(...)`. It will also help to maintain the consistency with the check in the function `checkValidity(...)`.

**Status**: Fixed

**Update from the client**: [Recommendation implemented.](https://github.com/worldcoin/worldcoin-grants-contracts/pull/7)

---