### This is a version of the report intended for publishing, simplified at the request of the team at Ramen Finance. Hyperlinks to snippets of code and to discussions were removed.

## Introduction & Scope

This audit was a two-part audit. The first part looks into the following contracts of ramen-finance: `GRamen.sol` and `Ramen.sol` as they were in the master branch the date 25th of April 2024.

The second part looks into V-68 with changes made to the previous contracts and the addition of the following contracts: `ClaimTimeLock.sol`, `DexManager.sol`, `DexTimelock.sol`, `Launchpad.sol`, `RamenProd.sol`, `TimelockFactory.sol` and `Vesting.sol`.

This audit was conducted by [kebabsec](https://twitter.com/kebabsec) members, [FlameHorizon](https://twitter.com/FlameHorizon1), [okkothejawa](https://twitter.com/okkothejawa), [shung](https://twitter.com/shunduquar), and [elcid](https://twitter.com/ElCid_eth).

****Note: This report does not provide any guarantee or warranty of security for the project.****

## Executive Summary

#### Table of Contents

* [Findings Part 1](#Part1)
1. [[INFORMATIONAL] Unnecessary setting of variable to `0`](#ISSUE1)
2. [[INFORMATIONAL] Unnecessary `nonReentrant` modifiers](#ISSUE2)
3. [[INFORMATIONAL] GRamen should inherit `ERC20`, instead of `ERC20Burnable`](#ISSUE3)
4. [[INFORMATIONAL] Floating pragma](#ISSUE4)
5. [[INFORMATIONAL] Unnecessary virtual modifiers](#ISSUE5)
6. [[INFORMATIONAL] Unnecessary checks](#ISSUE6)
7. [[INFORMATIONAL] `Stake` struct can fit into a single storage slot](#ISSUE7)
8. [[INFORMATIONAL] Redundant access control](#ISSUE8)
9. [[LOW] Increasing `startDecayTime` above `lockPeriod` will cause a panic underflow](#ISSUE9)
10. [[LOW] Penalty rate calculation should use `lockPeriod` at the time of staking, not its current value](#ISSUE10)

* [Findings Part 2](#Part2)
1. [[INFORMATIONAL] Unnecessary check in `mint` function](#ISSUEA1)
2. [[INFORMATIONAL] Duplicate checks present](#ISSUEA2)
3. [[INFORMATIONAL] Missing manager transfer function](#ISSUEA3)
4. [[INFORMATIONAL] Commented out code should be removed](#ISSUEA4)
5. [[INFORMATIONAL] Constructor parameter is not used](#ISSUEA5)
6. [[INFORMATIONAL] `safeTransferFrom` should be used instead of `transferFrom`](#ISSUEA6)
7. [[INFORMATIONAL] Unnecessary struct duplication](#ISSUEA7)
8. [[INFORMATIONAL] Repetitive reading from storage](#ISSUEA8)
9. [[INFORMATIONAL] Repetitive usage of calculation](#ISSUEA9)
10. [[LOW] Missing slippage check in `DexManager.sol`](#ISSUEA10)
11. [[LOW] Fluctuating fee may disable pool creation logic](#ISSUEA11)
12. [[LOW] Redundant `require` check](#ISSUEA12)
13. [[LOW] `cliff` duration is double counted](#ISSUEA13)
14. [[MEDIUM] `Release` function does not subtract release amount](#ISSUEA14)
15. [[HIGH] Refunds do not decrease relevant variables](#ISSUEA15)
16. [[HIGH] Function `launchProject` vulnerable to DoS attacks](#ISSUEA16)
17. [[CRITICAL] `TimeLock` struct can be overwritten by any user](#ISSUEA17)
18. [[CRITICAL] Malicious actor can cause unintended refunds](#ISSUEA18)
19. [[CRITICAL] Incorrect check could lead to unintended behaviour and vulnerability](#ISSUEA19)

<a id="summary"></a>

# Findings:

<a id="ISSUE1"></a>

### 1. [INFORMATIONAL] Unnecessary setting of variable to `0`

**Severity:** Informational

**Description:** When initializing the variable `penaltyRate`, there is no need to set it to `0`.

**Recommendation:** It is only necessary to initialize the variable.

<a id="ISSUE2"></a>

### 2. [INFORMATIONAL] Unnecessary `nonReentrant` modifiers

**Severity:** Informational

**Description:** The `nonReentrant` modifier is not needed, as the Ramen Contract code is known and it performs no external calls when doing transfers or burns.

**Recommendation:** Possibly remove the modifier.

<a id="ISSUE3"></a>

### 3. [INFORMATIONAL] GRamen should inherit `ERC20`, instead of `ERC20Burnable`

**Severity:** Informational

**Description:** The `_burn` internal function is available in OpenZeppelin's ERC20. ERC20Burnable only exposes these functions through external `burn` and `burnFrom` functions. In addition, GRamen disables `transfer` and `TransferFrom`, but these external burn functions from the ERC20Burnable extension can be used to bypass this restriction. These factors combined make it beneficial to use ERC20

**Recommendation:** Inherit ERC20 directly, instead of ERC20Burnable.

<a id="ISSUE4"></a>

### 4. [INFORMATIONAL] Floating pragma

**Severity:** Informational

**Description:** Using fixed pragma is beneficial to be able to always predict behaviour

**Recommendation:** Consider using fixed pragma.

<a id="ISSUE5"></a>

### 5. [INFORMATIONAL] Unnecessary virtual modifiers

**Severity:** Informational

**Description:** The virtual modifier is not needed.

**Recommendation:** Possibly remove the virtual modifier.

<a id="ISSUE6"></a>

### 6. [INFORMATIONAL] Unnecessary checks

**Severity:** Informational

**Description:** The checks "stake: insufficient RAMEN token" and "stake: insufficient allowance" are unnecessary.

**Recommendation:** Remove the checks.

<a id="ISSUE7"></a>

### 7. [INFORMATIONAL] `Stake` struct can fit into a single storage slot

**Severity:** Informational

**Description:** Since both `StakedAt` and `experiedAt` are uint64, changing `amount` to be uint128 would make it so the struct fits in a single storage slot.

**Recommendation:** Change the variable amount from uint256 to uint128.

<a id="ISSUE8"></a>

### 8. [INFORMATIONAL] Redundant access control

**Severity:** Informational

**Description:** The contract imports both AccessControl and Ownable2Step, and the latter isn't used. If the purpose is to have two step ownership, replacing AccessControl withAccessControlDefaultAdminRules would work.

**Recommendation:** Remove Ownable2Step and keep either AccessControl, or AccessControlDefaultAdminRules.

<a id="ISSUE9"></a>

### 9. [LOW] Increasing `startDecayTime` above `lockPeriod` will cause a panic underflow

**Severity:** Low

**Description:** Since there is no checks, the admin can change both `lockPeriod` and `startDecayTime`, if `startDecayTime` is changed to a number bigger than `lockPeriod`, it will cause a panic underflow in `calculatePenaltyRate` and break stake.

**Recommendation:** Add a check to prevent this case in the setters and in the constructor.

<a id="ISSUE10"></a>

### 10. [LOW] Penalty rate calculation should use `lockPeriod` at the time of staking, not its current value

**Severity:** Low

**Description:** If `lockPeriod` is changed after a user staked, penalty rate for unstaking will be calculated according to the current value of `lockPeriod` instead of the lock period user agreed to at the time of staking.

**Recommendation:** Calculate the penalty rate with `s.expiredAt` instead.

<a id="ISSUEA1"></a>

### A1. [INFORMATIONAL] Unnecessary check in `mint` function

**Severity:** Informational

**Description:** As the transaction would revert in `safeTransferFrom`, when `take` is called, this check is unnecessary.

**Recommendation:** Possibly remove the check.

<a id="ISSUEA2"></a>

### A2. [INFORMATIONAL] Duplicate checks present

**Severity:** Informational

**Description:** These lines contain the check `require(_p.addressOf.baseToken != address(0), "launchProject: base token address cannot be zero address");` twice.

**Recommendation:** Remove the duplicate check.

<a id="ISSUEA3"></a>

### A3. [INFORMATIONAL] Missing manager transfer function

**Severity:** Informational

**Description:** This snippet contains a function that would allow to transfer manager. Although it's purpose is just for testing, we do think it is a good idea to add a permissioned manager transfer function to the contract.

**Recommendation:** Possibly add a manager transfer function.

<a id="ISSUEA4"></a>

### A4. [INFORMATIONAL] Commented out code should be removed

**Severity:** Informational

**Description:** Across the codebase there were many cases of commented out code that was either for testing purposes or from previously used code.

**Recommendation:** Remove all the commented out code.

<a id="ISSUEA5"></a>

### A5. [INFORMATIONAL] Constructor parameter is not used

**Severity:** Informational

**Description:** The parameter `address owner` is not used.

**Recommendation:** Remove the parameter.

<a id="ISSUEA6"></a>

### A6. [INFORMATIONAL] `safeTransferFrom` should be used instead of `transferFrom`

**Severity:** Informational

**Description:** `transferFrom` is being used instead `safeTransferFrom`.

**Recommendation:** Replace `transferFrom` with `safeTransferFrom`.

<a id="ISSUEA7"></a>

### A7. [INFORMATIONAL] Unnecessary struct duplication

**Severity:** Informational

**Description:** It isn't necessary to re-create the exact same struct as the one in line 174 of Vesting.sol.

**Recommendation:** Set a variable to this struct and push that to the arrays instead.

<a id="ISSUEA8"></a>

### A8. [INFORMATIONAL] Repetitive reading from storage

**Severity:** Informational

**Description:** This function reads `received[_projectId][msg.sender]` from storage repetitively.

**Recommendation:** Instead, possibly cache it to a variable then read the relevant fields to save on storage reads.

<a id="ISSUEA9"></a>

### A9. [INFORMATIONAL] Repetitive reading from storage

**Severity:** Informational

**Description:** The calculation `p.presaleSold + p.contributeSold + p.overflowSold) - baseFee` is used both in line 178 and line 180.

**Recommendation:** Consider making this calculation a variable.

<a id="ISSUEA10"></a>

### A10. [LOW] Missing slippage check in `DexManager.sol`

**Severity:** Low

**Description:** Due to not having slippage checks, this code is vulnerable to sandwich attacks.

**Recommendation:** Consider adding slippage checks.

<a id="ISSUEA11"></a>

### A11. [LOW] Fluctuating fee may disable pool creation logic

**Severity:** Medium

**Description:** Uniswap v3 has four possible fee tiers, in the case of the `_poolFee` provided not being one of these tiers, pool creation logic may get temporarily disabled in the event of an invalid pool fee getting set.

**Recommendation:** Consider requiring this provided fee to be one of the tiers.

<a id="ISSUEA12"></a>

### A12. [LOW] Redundant `require` check

**Severity:** Low

**Description:** The check `_presaleReserved + _amount <= _presaleCap` in line 250 already ensures `_amount <= _presaleCap` so the part after the `&&` in line 248 is redundant.

**Recommendation:** Consider choosing between one of these.

<a id="ISSUEA13"></a>

### A13. [LOW] `cliff` duration is double counted

**Severity:** Low

**Description:** `cliff` is already included in `duration` and the calculation `_schedule.start + _schedule.cliff + _schedule.duration` is adding `cliff` with `duration`, therefore double counting `cliff`.

**Recommendation:** Remove `cliff` from the formula.

<a id="ISSUEA14"></a>

### A14. [MEDIUM] `Release` function does not subtract release amount

**Severity:** Medium

**Description:** The `allocations` function is responsible for checking (in lines 168 171 of Vesting.sol) if there's enough "unvested" `RAMEN` in the contract when creating a new schedule through the `createVestingSchedule` function. To do this it uses the variable `allVestingSchedules`, which is an array of `VestingSchedule` structs and subtracts `allVestingSchedules.amountTotal` from `ramen.balanceOf(address(this)`. When a schedule is created the new schedule is pushed (in lines 185-194 of Vesting.sol) into `allVestingSchedules`.

The problem is that when schedule is released through `release` and sends `RAMEN` to the staking contract, the release function does not subtract the release amount to `allVestingSchedules.amountTotal`. Eventually this will cause an underflow in `allocations` function since `allVestingSchedules[i].amountTotal` will always be bigger than `ramen.balanceOf(address(this)`, assuming that the amount of `ramen` transferred to `Vesting` will always be equal to the amount of the new schedule.

**Recommendation:** Subtract the release amount to `allVestingSchedules.amountTotal` as intended.

**Status:** Necessary subtraction is done now.

<a id="ISSUEA15"></a>

### A15. [HIGH] Refunds do not decrease relevant variables

**Severity:** High

**Description:** Refunds do not result in `presaleSold`, `contributeSold` and `overflowSold` being decremented even though the project does not have the associated funds anymore. This in some scenarios would cause the people buying after the funds are depleted to not be able to refund anymore.

With a 25% refund threshold, an attacker can buy 25% - 1 wei of the tokens, then refund, then buy 2 wei of the tokens to block the refund mechanism as threshold is met but with only 2 wei being sold in practice.

**Recommendation:** Decrement the appropriate variables in the case of a refund.

**Status**: Refund mechanism is now only limited to the period after project is concluded, so the attack scenario outlined above is not feasible anymore as an attacker cannot refund and then buy again.

<a id="ISSUEA16"></a>

### A16. [HIGH] Function `launchProject` vulnerable to DoS attacks

**Severity:** High

**Description:** It is possible for any user to DoS `launchProject` by front-running it with the exact same transaction but with zero as `baseSupply` and `airdropSupply`, as there is no checks on these values and would successfuly make a zero amount transfer which would most likely not revert.

**Recommendation:** Consider adding a check that prevents making these variables equal to zero.

**Status**: Launching a project requires a signature from Ramen Finance's own signer, and the signed message includes a specified caller address, so the function is not susceptible to frontrunning anymore.

<a id="ISSUEA17"></a>

### A17. [CRITICAL] `TimeLock` struct can be overwritten by any user

**Severity:** Critical

**Description:** Anyone can overwrite an existing `Timelock` struct by providing an existing `projectId` as input in the `projectId` parameter. This can be used to grief a project by overwriting this information with fake information, such as non existent ERC721 addresses which would result in the original not being able to be claim before rewriting the correct info.

**Recommendation:** Consider adding a check to prevent overwriting of `timelock` by any user.

**Status**: Function is access controlled and the path to call it do not allow for this kind of overwriting information attack.

<a id="ISSUEA18"></a>

### A18. [CRITICAL] Project tokens can be stolen by refunding malicious projects

**Severity:** Critical

**Description:** Consider the following scenario:
1. Wait for a project to launch with `launchProject`, and this would send some `baseToken` for this project to this contract.
2. Right after this legit project launch a malicious one with the same `baseToken`, then immediately trigger a refund by calling this `concludeProject`, as none of this project's token get sold a refund would be triggered. Then tokens of the legit project would be transferred to this new malicious project's treasury address.

**Recommendation:** Add a check to prevent projects with the same `baseToken` from interacting with each other.

**Status**: This is resolved as now each project has their own contract to store their `baseToken`'s instead of a shared one and this attack is mitigated due to this structural change.

<a id="ISSUEA19"></a>

### A19. [CRITICAL] Incorrect check could lead to overwriting project information

**Severity:** Critical

**Description:** The check in line 119 of Launchpad.sol is not a proper existence check and can lead to an issue in which an existing project's stored information would be overwritten with a fake one by inputting the same `_projectId`.

**Recommendation:** Replace the current check with `projects[_projectId].addressOf.baseToken == address(0)`.

**Status**: Proper check is added and launching a project now requires a signature from Ramen Finance's signer, thus this issue is mitigated.