# Quillhash Audit_mocks

**Repo:** https://github.com/Quillhash/Audit_mocks

https://github.com/Quillhash/Audit_mocks/blob/main/MockAccessControl.sol

## General

## `MockAccessControl.sol`

### [Critical] Function `pwn` can easily be hacked

**File(s)**: [`MockAccessControl.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MockAccessControl.sol)

**Description**: Since in the `pwn` function it is written as `tx.origin != msg.sender` which means only Contract Account can call this function. Next line it is written as `!isContract(msg.sender)` and in the `isContract` function we can see it checking the size of the caller and for `EOA` the size is 0 and for `Contract Account` it is greater than 0 and by that it is deciding whether it is a `EOA` or `Contract Account` but there is a flaw here because when the size is 0 there is also other case where the contract is calling the `pwn` function from the constructor and basically it is not at the runtime bytecode but it is at intial bytecode so thats why the size returned will be equal to 0. The last piece of code which can be hacked it is`block.timestamp % 120 >= 0 && block.timestamp % 120 < 60` in `pwn` function because block.timestamp is a deterministic and thats the reason using the External function `timeVal` we can actually get the `block.timestamp` value and hack the piece of code.

**Recommendation(s)**: If you want to only a `EOA` then use the code as `tx.origin == msg.sender` and if you only want a `Contract Account` then use `tx.origin != msg.sender`. For randomness never use global like `block.timestamp` instead use `Chainlink VRF` for random number generator.

---
### [Info] Use `Call` instead of `Transfer` for transferring Ether

**File(s)**: [`MockAccessControl.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MockAccessControl.sol)

**Description**: In this `retrieve` function the `transfer` function is used to send the ether which has a gas limit of 2300 and generally there is a gas limit to avoid reentrancy hacks but the problem is this `transfer` function can send the ether to a `EOA` within the gas limit but it will fail to send it to a `Contract Account`.

**Recommendation(s)**: Use `Call` instead of `Transfer` for transferring Ether and to avoid reentrancy attacks use a higher gas limit and also use a reentrancy lock to avoid these type of attacks.

---

### [Best Practices] Avoid using strings with length greater than 32 bytes

**File(s)**: [`MockAccessControl.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MockAccessControl.sol)

**Description**: The `require(...)` string in the Line 23,24,25,26,43 are having more than 32 bytes.

**Recommendation(s)**: Consider reducing the length of this string up to 32 bytes or else revert and error can be used instead to save gas.

---
### [Best Practices] Use != 0 instead of > 0 for unsigned integer comparison

**File(s)**: [`MockAccessControl.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MockAccessControl.sol)

**Description**: In the Line 43 the variable is compared as >0.

**Recommendation(s)**: Consider changing >0 to !=0 to save gas.

---

### [Best Practices] Use assembly to check for `address(0)`

**File(s)**: [`MockAccessControl.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MockAccessControl.sol)

**Description**: In the Line 37 the variable is compared as `!= address(0)`

**Recommendation(s)**: Use assembly to check for `address(0)`

---

### [Best Practices] Using bools for storage incurs overhead

**File(s)**: [`MockAccessControl.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MockAccessControl.sol)

**Description**: In the Line 6 the mapping output is used as boolean `mapping(address => bool) private pwned;` and which costs a lot of gas.

**Recommendation(s)**: Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past.
Change the above piece of code to `mapping(address => uint256) private pwned;`

---