# ICO Audit
Commit hash link:
https://github.com/0xMacro/student.morganjweaver/tree/19e6c24efe40554db87a8578dbe5e1b9b695e26f
Audited by: Parth Patel (Parth#7949)
# Issues
**[H-1]** Total contribution limit in the `SEED` and `GENERAL` phases isn't taken care of.
On [line 63-86](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/Ico.sol#L63-L86), Ico.sol has following code:
```solidity
function contribute() external payable {
if(isPaused) revert ContributionsCurrentlyPaused();
if(!isAllowed(msg.sender)) revert NotAllowed();
if(goal - totalRaised == 0) revert GoalIsMet();
if(msg.value > remainingContributionInPhase(msg.sender)) revert ContributionLimitMet();
PhaseContributions storage individualContrib = contributions[msg.sender];
if(individualContrib.initialized == false){
PhaseContributions memory newContributor = PhaseContributions(0,0,0,0, true);
contributions[msg.sender] = newContributor;
individualContrib = contributions[msg.sender]; // superfluous?
}
if (phase == Phase.SEED){
individualContrib.seed += msg.value;
} else if (phase == Phase.GENERAL) {
individualContrib.general += msg.value;
} else if (phase == Phase.OPEN) {
individualContrib.open += msg.value;
} else {
revert PhaseStateError();
}
totalRaised += msg.value;
emit Contribution(msg.sender);
}
```
The users can contribute more than total allowed contribution in the `SEED` phase and `GENERAL` phase. The `SEED` phase has a total contribution limit of `15000 ether` and the `GENERAL` phase has a total contribution limit of `30000 ether`. The test demonstrating this vulnerability is as follows:
```typescript
it("total contributions during SEED phase exceeding exceed 15000 ether", async () => {
const { ico, spaceCoin, deployer, alice, bob, treasury } =
await loadFixture(setupFixture);
const extraAccounts: Array<SignerWithAddress> = await ethers.getSigners();
const extraAddresses: Array<string> = extraAccounts.map(
(item) => item.address
);
const privateContributorsAccounts: Array<SignerWithAddress> = [
...extraAccounts,
alice,
bob,
];
const privateContributorsWithAddresses: Array<string> = [
...extraAddresses,
alice.address,
bob.address,
];
for (let i = 0; i < privateContributorsWithAddresses.length; i++) {
await ico
.connect(deployer)
.allowList(privateContributorsWithAddresses[i]);
}
for (let i = 0; i < 29; i++) {
await ico
.connect(privateContributorsAccounts[i])
.contribute({ value: ONE_ETHER.mul(1000) });
}
const contributor: SignerWithAddress = privateContributorsAccounts[31];
expect(await ico.totalRaised()).equal(ONE_ETHER.mul(29000));
expect(
await ico
.connect(contributor)
.contribute({ value: ONE_ETHER.mul(1200) })
).to.be.ok;
//it is possible to exceed the total contribution limit of 15000 ether in the SEED phase
expect(await ico.totalRaised()).equal(ONE_ETHER.mul(30200));
});
```
Consider: adding a check for whether the total contribution limit isn't reached with the contribution. Pseudo-code for `SEED` phase below:
```solidity
require(totalRaised + msg.value <= 15_000 ether,
"seed phase - total contribution limit exceeded");
```
**[H-2]** ICO contributors will be able to redeem their SPC tokens even though Contribution and SPC redemption is paused. On [line 120-131](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/Ico.sol#L120-L131), Ico.sol has following code:
```solidity
function redeem(uint256 amount) public {
if(phase != Phase.OPEN) revert NotAllowed();
PhaseContributions storage individual = contributions[msg.sender];
uint256 totalContrib = individual.general + individual.open + individual.seed;
if (totalContrib == 0) revert NoContributions();
if (amount > totalContrib - individual.redeemed) revert AlreadyRedeemed();
individual.redeemed += amount;
spaceCoin.transfer(msg.sender, amount*spacToEthRatio);
emit Redeemed(msg.sender, amount);
}
```
The above function(i.e. `redeem`) doesn't check whether the contribution and SPC redemption are paused or not allowing contributors to withdraw their SPC tokens in the `OPEN` phase even if `isPaused` is `true`.
Consider: adding a check on `isPaused` and revert if it is true.
Fixed code:
```solidity
function redeem(uint256 amount) public {
if (isPaused) revert ContributionsCurrentlyPaused();
if(phase != Phase.OPEN) revert NotAllowed();
PhaseContributions storage individual = contributions[msg.sender];
uint256 totalContrib = individual.general + individual.open + individual.seed;
if (totalContrib == 0) revert NoContributions();
if (amount > totalContrib - individual.redeemed) revert AlreadyRedeemed();
individual.redeemed += amount;
spaceCoin.transfer(msg.sender, amount*spacToEthRatio);
emit Redeemed(msg.sender, amount);
}
```
**[L-1]** Owner can accidentally advance phase two times and can't move back to previous phase.
On [line 48-52](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/Ico.sol#L48-L52), Ico.sol has following code:
```solidity
function advancePhase() external onlyOwner {
if(phase == Phase.OPEN) revert InFinalPhase();
phase = Phase(uint(phase) +1);
emit PhaseAdvanced(phase);
}
```
It is possible for the owner to accidentally send two transactions in a row and advance the phase two times that may not be desired. Such a situation can arise if the first transaction is not mined and the owner send the other transaction or maybe because of some issues with the frontend of Dapp.
Consider: adding an argument in the function which can be desired phase that the owner wants to go to. This will prevent the accidently skipping of phase.
Fixed code:
```solidity
error InvalidPhaseTransition();
function advancePhase(uint8 desiredPhase) external onlyOwner {
if(phase == Phase.OPEN) revert InFinalPhase();
if (desiredPhase != uint8(phase) + 1) revert InvalidPhaseTransition();
phase = Phase(desiredPhase);
emit PhaseAdvanced(phase);
}
```
**[L-2]** Transaction may revert due to underflow
On [line 66](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/Ico.sol#L66), Ico.sol has following code:
```solidity
if(goal - totalRaised == 0) revert GoalIsMet();
```
`goal - totalRaised` can underflow because of the issue described in **[H-1]** because it is possible to exceed total contribution limit in `SEED` and `GENERAL` phases. Consider the case when `totalRaised` is `31000 ether`. Now, `goal` will be fixed and `30000 ether`. So, the subtraction will underflow and the transaction is reverted which is not expected.
Consider: Fixing the bug in `H-1` will fix this too.
The test describing this bug is as follows:
```typescript
it("transaction reverts due to underflow", async () => {
const { ico, spaceCoin, deployer, alice, bob, treasury } =
await loadFixture(setupFixture);
const extraAccounts: Array<SignerWithAddress> = await ethers.getSigners();
const extraAddresses: Array<string> = extraAccounts.map(
(item) => item.address
);
const privateContributorsAccounts: Array<SignerWithAddress> = [
...extraAccounts,
alice,
bob,
];
const privateContributorsWithAddresses: Array<string> = [
...extraAddresses,
alice.address,
bob.address,
];
for (let i = 0; i < privateContributorsWithAddresses.length; i++) {
await ico
.connect(deployer)
.allowList(privateContributorsWithAddresses[i]);
}
for (let i = 0; i < 29; i++) {
await ico
.connect(privateContributorsAccounts[i])
.contribute({ value: ONE_ETHER.mul(1000) });
}
const contributor: SignerWithAddress = privateContributorsAccounts[31];
expect(await ico.totalRaised()).equal(ONE_ETHER.mul(29000));
expect(
await ico
.connect(contributor)
.contribute({ value: ONE_ETHER.mul(1200) })
).to.be.ok;
expect(await ico.totalRaised()).equal(ONE_ETHER.mul(30200));
// Below line will revert due to underflow because goal is 30000 ether and totalRaised is 30200 ether.
await ico.connect(contributor).contribute({ value: ONE_ETHER.mul(1200) });
});
```
**[L-3]** SpaceCoin contract not verified on etherscan.
In the `README.md`, SpaceCoin contract address is `0x5d188a4c1E4C6dd523aabfffdBA32eb23c8305fA`. The contract can be found [here](https://goerli.etherscan.io/address/0x5d188a4c1E4C6dd523aabfffdBA32eb23c8305fA#code) and it is not verified.
Consider: verifying the SpaceCoin contract.
# Nitpicks
## Ico.sol
**[Q-1]** Adding a large list of private investors in `allowList` will require a large number of transactions.
On [line 116-118](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/Ico.sol#L116-L118), Ico.sol has following code:
```solidity
function allowList(address contributor) external onlyOwner {
seedAllowlist[contributor] = 1;
}
```
To add one contributor to a list, it needs to be passed in `allowList`. So, to add `X` number of contributors, `X` transactions are required. The name of the method is `allowList` but it is allowing only single contributor which is also to be noted.
Consider: passing the array of contributors that needs to be allowed and emitting the event.
Fixed code:
```solidity
event PrivateContributorsAdded(address[] contributors);
function allowList(address[] calldata contributors) external onlyOwner {
for (uint256 i = 0; i < contributors.length; i++) {
seedAllowlist[contributors[i]] = true;
}
emit PrivateContributorsAdded(contributors);
}
```
**[Q-2]** Some variables are storage variables even though they can be set as `constant` or `immutable`.
On [line 15-16](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/Ico.sol#L15-L16), Ico.sol has following code:
```solidity
address public owner;
address public treasury;
```
`owner` and `treasury` are only set in the constructor and are not intended to change later. So, it shouldn't be stored in storage due to the expensiveness of storage.
Consider: changing both the variables to immutable. Also, the convention of the name of constant and immutable variables is upper case.
Fixed code:
```solidity
address public immutable OWNER;
address public immutable TREASURY;
```
**[Q-3]** `uint8` is used instead of `bool` which may result in the setting of undesired values except 0(false) and 1(true).
On [line 23](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/Ico.sol#L23), Ico.sol has following code:
```solidity
mapping(address => uint8) seedAllowlist;
```
In the code, `seedAllowlist` of any address is compared against `0` and `1` which stands for `false` and `true` respectively. These variables are intended to use for bool but have a datatype of `uint8`. This may result in this variable taking unwanted values except `0` or `1`.
Consider: changing the data type to `bool`.
Fixed code:
```solidity
mapping(address => bool) seedAllowlist;
```
**[Q-4]** Initialization of storage variables to default values.
On [line 43-46](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/Ico.sol#L43-L45) in the constructor, Ico.sol has following code:
```solidity
phase = Phase.SEED;
isPaused = false;
totalRaised = 0;
```
`phase` is set to a value of 0 which is its default value. `isPaused` is set to false which is its default value and `totalRaised` is set to 0 which is also its default value.
Consider: leaving the variable as it is because they will be the same after initialization.
**[Q-5]** `setPauseBool` can be set to `true` even when it is `true` or can be set to `false` even when it is `false`
On [line 54-58](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/Ico.sol#L54-L58), Ico.sol has following code:
```solidity
function setPauseBool(bool pauseOn) external onlyOwner {
isPaused = pauseOn;
emit Paused();
}
```
The function doesn't check if it is updating to the same value. This will result in emitting the `Paused` event every time causing the off-chain clients listening to these events to misinterpret the information.
Also, the naming of the event emitted is confusing because it will be emitted even though isPaused will be set to false. When contributions and SPC redemptions are resumed, emitting the `Paused` event doesn't make sense.
Consider: renaming the name of the event emitted and checking whether the value to be set is the same as the current value before updating.
Fixed code:
```solidity
error SameValueToBeSet();
event SpcRedemptionAndContributionPauseToggled();
function setPauseBool(bool pauseOn) external onlyOwner {
if (isPaused == pauseOn) revert SameValueToBeSet();
isPaused = pauseOn;
emit SpcRedemptionAndContributionPauseToggled();
}
```
**[Q-6]** `redeem` method can be made `external` instead of public.
On [line 120](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/Ico.sol#L120), Ico.sol has following code:
```solidity
function redeem(uint256 amount) public {
```
This method's visibility can be changed from public to external which will result in gas savings. Also, this method is not supposed to be called `internally`.
Consider: changing the visibility to `external`.
Fixed code:
```solidity
function redeem(uint256 amount) external {
```
**[Q-7]** Adding a parameter of `amount` contributed can be helpful for off-chain clients in `Contribution` event.
On [line 85](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/Ico.sol#L85), Ico.sol has following code:
```solidity
emit Contribution(msg.sender);
```
Adding another parameter of an amount to this event can be useful for off-chain clients which may be tracking total contributions and how much contribution has each contributor done.
Consider: adding another parameter in event `Contribution`.
Fixed code:
```solidity!
event Contribution(address indexed contributor, uint256 contribution);
emit Contribution(msg.sender, msg.value);
```
## SpaceCoin.sol
**[Q-8]** Redundant creation of mint function.
On [line 36-38](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/SpaceCoin.sol#L36-L38), SpaceCoin.sol has following code:
```solidity
function mint(address to, uint256 amount) public onlyOwner {
_mint(to, amount);
}
```
The function calls the internal function `_mint` with the same arguments. Both function i.e. `mint` and `_mint` takes the same arguments which make the public `mint` function redundant. Also, `totalSupply` of SPC tokens needs to be capped at `500_000` and since `500_000` SPC tokens are minted in the constructor, there is no need for the public `mint` function.
Consider: Removing the function.
**[Q-9]** `taxesOn` can be set to `true` even when it is `true` or can be set to `false` even when it is `false`
On [line 31-34](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/SpaceCoin.sol#L31-L34), SpaceCoin.sol has following code:
```solidity
function taxSwitch(bool _taxesOn) external onlyOwner {
taxesOn = _taxesOn;
emit TaxToggle(taxesOn);
}
```
The function doesn't check if it is updating to the same value. This will result in emitting the `TaxToggle` event every time causing the off-chain clients listening to these events to misinterpret the information.
Consider: adding a check for whether the value to be set is the same as the current value before updating.
Fixed code:
```solidity
error SameValueToBeSet();
function taxSwitch(bool _taxesOn) external onlyOwner {
if (taxesOn == _taxesOn) revert SameValueToBeSet();
taxesOn = _taxesOn;
emit TaxToggle(taxesOn);
}
```
**[Q-10]** Unused variable `maxSupply`.
On [line 11](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/SpaceCoin.sol#L11), SpaceCoin.sol has following code:
```solidity
uint256 constant maxSupply = 500000;
```
The above variable is not used anywhere and can be omitted.
Consider: removing the variable.
**[Q-11]** Constants should be named with all capital letters with underscores separating words.
On [line 11](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/SpaceCoin.sol#L8), SpaceCoin.sol has following code:
```solidity
uint256 constant taxPercent = 2;
```
According to Solidity [style guide](https://docs.soliditylang.org/en/v0.8.16/style-guide.html#constants), constants should be named with all capital letters.
Consider: renaming the variable.
Fixed code:
```solidity
uint256 constant TAX_PERCENT = 2;
```
**[Q-12]** Pseudocode not readable in design exercise answer in `README.md`.
[Link](https://github.com/0xMacro/student.morganjweaver/tree/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico#design-exercise-answer)
Consider: wrapping the code in code blocks in `README.md`.
**[Q-13]** Code indentation could have been made better.
There are small issues with code indentation.
[Example 1](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/Ico.sol#L88) There is a space between the opening brace(`{`) and ending brace of function args(`)`)
[Example 2](https://github.com/0xMacro/student.morganjweaver/blob/19e6c24efe40554db87a8578dbe5e1b9b695e26f/ico/contracts/Ico.sol#L103) There is no space between the opening brace(`{`) and ending brace of function args(`)`)
Consider: using a linter to format code.
Sign in with Wallet
Connect another wallet