# NM-0066 Sentinel

Repo: [LINK TO BE ADDED]

Blob: [LINK TO BE ADDED HERE]

## General

### (Prosper) [Best Practice and Gas Optimization] Struct Packed I

**File(s)**: [`contractrow.sol`]

**Description**:
The struct currently occupy four slots, if packed tightly, Three slots should serve for it intended purpose.

 struct Fill {
        address investor;
        uint256 amountFilled;
        bool isVested;
        uint256 vestDeadline;
    }

Address on EVM only has 20 bytes, why bool has just one bytes.

**Recommendation(s)**:
Consider using a well packed storage

  struct Fill {
        address investor; // 20 bytes
        bool isVested; // takes in 1 bytes
        uint256 amountFilled;
        uint256 vestDeadline;
    }

**Status**: Unresolved

**Update from the client**:

---

### (Prosper) [Best Practice and Gas Optimization] Struct Packed II

**File(s)**: [`contracts/ManualEscrow.sol`]

**Description**: The Order struct can also be optimized and packed

 struct Order {
    uint256 availableAmount;
    uint256 withdrawableAmount;
    address paymentToken;
    address owner;
    uint256 fillDeadline;
    // stores the fees set when order was created
    uint16 feeBps;
    Fill[] fills;
}

Address on EVM only has 20 bytes, why uint16 has just two bytes.

**Recommendation(s)**:
Consider using a well packed storage

  struct Order {
    uint256 availableAmount;
    uint256 withdrawableAmount;
    uint256 totalDeposit; // new variable introduced
    address paymentToken;
    address owner; // 20 bytes
    // stores the fees set when order was created
    uint16 feeBps; // 2 bytes
    uint256 fillDeadline;
    Fill[] fills;
}

**Status**: Unresolved

**Update from the client**:

---

### (Prosper) [Info] Removal of Redundant Mapping

**File(s)**: [`contracts/ManualEscrow.sol`]

**Description**: The mapping with the name `fillsByOrderId` is declared within the contract and is never used.

 mapping(uint256 => Fill[]) public fillsByOrderId;

**Recommendation(s)**:
Consider removing redundant codes to reduce deployment cost

**Status**: Unresolved

**Update from the client**:

---

### (Prosper) [Best Practice] Missing Indexed param on Event

**File(s)**: [`contracts/ManualEscrow.sol`]

**Description**: The Nature and designed pattern of this contract has a lot to do with offchain.

 event OrderFilled(uint256 orderId, address investor, uint256 amount, uint256 fillIndex);

**Recommendation(s)**:
consider adding an `indexed` keyword in required places in event, will allow you to search for these events using the indexed parameters as filters.

 event OrderFilled(uint256 orderId, address indexed investor, uint256 amount, uint256 fillIndex);
// and
event OrderCreated(uint256 orderId, address owner, address indexed paymentToken, uint256 amount);

**Status**: Unresolved

**Update from the client**:

---

### (Prosper) [Low] fill Deadline Checks

**File(s)**: [`contracts/ManualEscrow.sol`]

**Description**: There is no check in the `createOrder` function to ensure that the `_fillDeadline` must not be less than the current block.timestamp, the impart of this is that paradventure the project owner mistakenly set a time less than the current timestamp Investors will not be able to invest on the orderID

**Recommendation(s)**:
We can save ourself from this by adding the following line

 require(_fillDeadline > block.timestamp, "Invalid time");

**Status**: Unresolved

**Update from the client**:

---

### (Prosper) [Medium] fill Deadline Checks

**File(s)**: [`contracts/ManualEscrow.sol`]

**Description**: The contract function in a way that Investors can fill an orderwith an amount greater than the amount that the project owner set, which is outside the scope of the project.
The impart of this is that during releaseFills, and the amount filled is above `order.availableAmount` the transaction will fail

function fillOrder(uint256 _orderId, uint256 _amountToFill) external {}

There is no check to ensure that `_amountToFill` is less than `order.availableAmount` which is the available and expected amount specified by the owner.

**Recommendation(s)**:
This check can be included.

function fillOrder(uint256 _orderId, uint256 _amountToFill) external {
    uint256 newSumAmount = order.totalDeposit + _amountToFill;
    require(newSumAmount < order.availableAmount, "M: Amount Exceed available amount");
}

**Status**: Unresolved

**Update from the client**:

---

### (Olivier) [INFO] TODOs left in the code

**File(s)**: [`contracts/ManuelEscrow.sol`, `contracts/Vesting.sol`]

**Description**: TODOs left in the code

**Status**: Unresolved

**Update from the client**:

---

### (Olivier) [GAS] Use unchecked {++i} in loops

**File(s)**: [`contracts/ManualEscrow.sol`]

**Description**: Use unchecked { ++i; } at the end of the for loop in releaseFills to save some gas.

Gas usage of releaseFills before (Min: 81913, Max: 103261) => after (Min: 81842, Max: 103119) (as reported by gas-reporter)

**Status**: Unresolved

**Update from the client**:

---

### (Olivier) [GAS] Use assembly to write storage value

**File(s)**: [`contracts/ManualEscrow.sol`]

**Description**: Line 225 in ManualEscrow.sol, instead of "feeReceiver = _address;", use assembly {sstore(feeReceiver.slot, _address)} to save gas.

Gas usage of editFeeReceiver before (Avg: 30812) => after (Avg: 30773) (as reported by gas-reporter)

**Status**: Unresolved

**Update from the client**:

---

### (Olivier) [GAS] Use custom errors rather than require() string to save gas

**File(s)**: [`contracts/*`]

**Description**:

Custom errors are available from solidity version 0.8.4.
Custom errors save ~50 gas each time they’re hit by avoiding having to allocate and store the revert string.
Not defining the strings also save deployment gas.

**Status**: Unresolved

**Update from the client**:

---

### (Olivier) [GAS] Use storage instead of memory for some structure.

**File(s)**: [`contracts/vesting.sol`]

**Description**:

Storage pointer to a structure is cheaper than copying each value of the structure into memory.
It may not be obvious, but every time you copy a storage struct/array/mapping to a memory variable,
you are copying each member by reading it from storage, which is expensive.
And when you use the storage keyword, you are just storing a pointer to the storage, which is much cheaper.

Replace 'Schedule memory _schedule' with 'Schedule storage _schedule' line 359 of Vesting.sol, in the investorDeposit().

Gas usage of investorDeposit before (Min: 121322, Max: 194501, Avg: 169884) => after (Min: 119074, Max: 191793, Avg: 167620) (as reported by gas-reporter)

**Status**: Unresolved

**Update from the client**:

---

### (Olivier) [GAS] Do not use '+=' for state variables.

**File(s)**: [`contracts/vesting.sol`]

**Description**:

Found line 287 in Vesting.sol). Instead use "paymentTokensAccrued = paymentTokensAccrued + paymentTokenAmount;" in order to save some gas.

Gas usage of investorDeposit before (Min: 121322, Max: 194501, Avg: 169884) => after (Min: 121306, Max: 194485, Avg: 169868) (as reported by gas-reporter)

**Status**: Unresolved

**Update from the client**:

---

### (Olivier) [GAS] Use payable method

**File(s)**: [`contracts/*.sol`]

**Description**:

Use payable method on function that will always revert when called by user.
If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function.
Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.
The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

exemple: Gas usage of startVesting before using payable (Min: 331378, Max: 531633, Avg: 371748) => after (Min: 331354, Max: 531609, Avg: 371724) (as reported by gas-reporter)

**Status**: Unresolved

**Update from the client**:

---