Commit hash link: https://github.com/0xMacro/student.Rahat-ch/tree/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0

Audited by: Parth Patel (Parth#7949)
# General Comments
There are no major bugs or vulnerabilities. `checks-effects-interactions` pattern is used which prevents reentrancy.
# Issues
**[M-1]** Goal of the project should be more than or equal to 0.01 ether, otherwise project won't be able to accept any contributions.

On [line 21-25](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/Project.sol#L21-L25), Project.sol has the following code:

constructor(uint256 _goal, address _owner, uint256 _expiration, string memory _title, string memory _tokenSymbol)ERC721(_title, _tokenSymbol){
goal = _goal;
owner = _owner;
expiration = _expiration;
}

If the `_goal` is less than `0.01 ether`, the project will be successfully created and no one will be able to contribute to that project.

Consider: adding require statement to check whether `_goal` is more than or equal `0.01 ether`.

Fixed code:

constructor(uint256 _goal, address _owner, uint256 _expiration, string memory _title, string memory _tokenSymbol)ERC721(_title, _tokenSymbol){
        require(_goal >= 0.01 ether, "Invalid goal");
        goal = _goal;
        owner = _owner;
        expiration = _expiration;
    }

**[Q-1]** no mention of force-fed ether in the specs and force-fed ether is not taken into the contribution

On [line 37-39](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/Project.sol#L37-L39), Project.sol has following code:

 if(address(this).balance >= goal){
            goalMet = true;
        }

This will consider force-fed ether towards the contribution. The ether can be force-fed to the contract as mentioned in this [consensys article](https://consensys.github.io/smart-contract-best-practices/attacks/force-feeding/).

Consider: Using a separate variable for tracking contributions received through `contribute` function and using the checks against that variable.

**[Q-2]** lack of a test coverage report in `README.md`

The test coverage report is not added in the `README` file.
The test coverage report on running `npx hardhat coverage` is as follows:

File | % Stmts | % Branch | % Funcs | % Lines |Uncovered Lines |
---------------------|----------|----------|----------|----------|----------------|
contracts/ | 100 | 93.75 | 100 | 100 | |
Project.sol | 100 | 93.75 | 100 | 100 | |
ProjectFactory.sol | 100 | 100 | 100 | 100 | |
All files | 100 | 93.75 | 100 | 100 | |

# Nitpicks

## ProjectFactory.sol

**[Q-1]** Not `indexed` on owner in the `ProjectCreated` event.

On [line 6](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/ProjectFactory.sol#L6), ProjectFactory.sol has the following code:

event ProjectCreated(address indexed newProject, address owner, uint256 goal, string title, string tokenSymbol);

If the field of `owner` is made `indexed`, it will be easier to query all the Projects created by a particular owner off-chain.

Consider: Making `owner` indexed in the event emitted.

Fixed code:

event ProjectCreated(address indexed newProject, address indexed owner, uint256 goal, string title, string tokenSymbol);

**[Q-2]** Use of magic value ( i.e. 30 days)

On [line 9](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/ProjectFactory.sol#L9), ProjectFactory.sol has the following code:

Project project = new Project(_goal, msg.sender, block.timestamp + 30 days, _title, _tokenSymbol);

Consider: Consider adding a `constant` variable that stores this magic value. `constant` variables don't occupy storage slot and can't be rewritten, so there won't be extra gas cost.

## Project.sol

**[Q-3]** Inconsistent emitting of events across the code

Consider the below functions in `Project.sol`

[contribute function](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/Project.sol#L32-L50)

[withdraw function](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/Project.sol#L52-L59)

[getRefund function](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/Project.sol#L61-L70)

[cancelProject function](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/Project.sol#L72-L76)

`contribute` function emits the event before external call following `checks-effects-interactions` pattern while `withdraw`, `getRefund` and `cancelProject` emits the event after external call and doesn't follow `checks-effects-interactions` pattern.

Consider: emitting events before external calls and follow `checks-effects-interactions` pattern.

Fixed code:

event ProjectCreated(address indexed newProject, address indexed owner, uint256 goal, string title, string tokenSymbol);

**[Q-4]** Not use of `immutable` keyword for a variable that is only stored once in the constructor.

On [line 7-9](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/Project.sol#L7-L9), Project.sol has the following code:

uint256 public goal;
address public owner;
uint256 public expiration;

If the variable `goal`, `owner` and `expiration` is made of the `immutable` type, it won't hold a slot in storage and is also cheaper to write. `immutable` keyword guarantees that the variable can't be rewritten after construction of contract.

Consider: Making the above variables(i.e.`goal`, `owner` and `expiration` of immutable type).

Fixed code:

uint256 public immutable GOAL;
address public immutable OWNER;
uint256 public immutable EXPIRATION;

**[Q-5]** `tokenId` starts from zero for NFTs.

On [line 12](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/Project.sol#L12), Project.sol has the following code:

uint256 public tokenId = 0;

In some popular projects, `tokenId` 0 is not supposed to be minted by user. It will be weird for the user to own `tokenId: 0` and using functions like `tokenByIndex` for `enumerable ERC721` contracts which will tell them they own token 0 causing confusion. More discussion can be seen on [OpenZeppelin forum](https://forum.openzeppelin.com/t/are-nft-projects-doing-starting-index-randomization-and-provenance-wrong-or-is-it-just-me/14147/5)

Consider: Initializing `tokenId` from 1.

Fixed code:

    uint256 public tokenId = 0;

**[Q-6]** Arg of event emitted is always `true`.

On [line 19](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/Project.sol#L19), Project.sol has the following code:

event ProjectCancelled(bool isCancelled);

The above event is only emitted once when the project is canceled i.e. on [line 75](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/Project.sol#L75) of Project.sol.

emit ProjectCancelled(true);

If the event `ProjectCancelled` is emitted, then it's arg will always be `true`. So, there is no benefit to off-chain components consuming this events.

Consider: emitting the event with `time` at which the Project is canceled.

Fixed code:

event ProjectCancelled(uint256 time);

Emitting the event:

event ProjectCancelled(block.timestamp);

**[Q-7]** `10 ** 18` is used instead of `1 ether` and indent changes is required.

On [line 41](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/Project.sol#L41), Project.sol has the following code:

uint256 amountToMint =contributorToContribution[msg.sender]/10 ** 18 - contributorTokenAmount[msg.sender];

Consider: using `1 ether` instead of `10 ** 18` and putting space after `=` for better readability.

Fixed code:

uint256 amountToMint = contributorToContribution[msg.sender]/1 ether - contributorTokenAmount[msg.sender];

**[Q-8]** Redundant check for contribution while claiming refund of the contributors.

On [line 62](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/Project.sol#L62), Project.sol has the following code:

require(contributorToContribution[msg.sender] >= 0.01 ether, "No contributions available for withdrawal");

The above line is not required since contribution is only considered when it is greater than `0.01 ether`. For reference, see [line 33](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/contracts/Project.sol#L33)

Consider: removing the check on line 62

## **[Q-9]** Pass a "to" address paramater to the `claimProjectFund` function

On [line 52](https://github.com/0xMacro/student.Rahat-ch/blob/master/crowdfund/contracts/Project.sol#L52), Project.sol has following code:

function withdraw(uint256 amountToWithdraw) external onlyOwner {
require(activeProject, "This project has failed");
require(goalMet, "Goal has not been met yet");
require(amountToWithdraw <= address(this).balance, "You do not have sufficent funds to withdraw");
(bool sent,) = msg.sender.call{value: amountToWithdraw}("");
require(sent, "Transfer failed");
emit WithdrawalMade(amountToWithdraw, address(this).balance);
}

The above code doesn't allow the owner to withdraw the funds to other address. If the owner want to move the funds to other address, he first needs to withdraw it and get the funds into his account and then sending the funds again to desired address. For this, event parameters of `WithdrawalMade` on [line 17](https://github.com/0xMacro/student.Rahat-ch/blob/master/crowdfund/contracts/Project.sol#L17) is recommended.

Consider: adding a `to` parameter (of type `address`) to the function signature, so the creator can withdraw their funds to a different address, calling `payable(to)` instead.

Fixed code:

function withdraw(address to, uint256 amountToWithdraw) external onlyOwner {
        require(activeProject, "This project has failed");
        require(goalMet, "Goal has not been met yet");
        require(amountToWithdraw <= address(this).balance, "You do not have sufficent funds to withdraw");
        (bool sent,) = to.call{value: amountToWithdraw}("");
        require(sent, "Transfer failed");
        emit WithdrawalMade(to, amountToWithdraw, address(this).balance);

**[Q-10]** 2 tests failing for Project.sol

On [line 406-423](https://github.com/0xMacro/student.Rahat-ch/blob/6ef45b4d73a8f493628c6be04a1e5e4661ccfda0/crowdfund/test/crowdfundr.test.ts#L406-L423), crowdfundr.test.ts has the following code:

it("Awards a contributor with a second badge when their total contribution to a single project sums to at least 2 ETH", async () => {
const { project, bob } = await loadFixture(setupFixture);
await project.connect(bob).contribute({ value: ONE_ETHER });
await project.connect(bob).contribute({ value: ONE_ETHER });
const tokenOwner = await project.ownerOf(BigNumber.from(1));
const secondTokenOwner = await project.ownerOf(BigNumber.from(2));
expect(tokenOwner).to.equal(bob.address);
expect(secondTokenOwner).to.equal(bob.address);
});

it("Does not award a contributor with a second badge if their total contribution to a single project is > 1 ETH but < 2 ETH", async () => {
const { project, bob } = await loadFixture(setupFixture);
await project.connect(bob).contribute({ value: ONE_ETHER });
await project.connect(bob).contribute({ value: ethers.utils.parseEther(".5")});
const tokenOwner = await project.ownerOf(BigNumber.from(1));
expect(tokenOwner).to.equal(bob.address);
await expect(project.ownerOf(BigNumber.from(2))).to.be.revertedWith("ERC721: invalid token ID");
});

The tests are failing because the project is initialized with `1 ether` and after reaching the goal, contributing again will revert.

Consider: using the Project with the goal of more than or equal to `2 ether` and `1.5 ether` for the first and second test respectively.