# Quiz

{% embed url="<https://ventral.digital/posts/2021/11/7/secureum-bootcamp-security-pitfalls-amp-best-practices-101-quiz>" %}

**Q1 The use of pragma in the given contract snippet**

```solidity
pragma solidity ^0.6.0;

contract test {
   // Assume other required functionality is correctly implemented
   // Assume this contract can work correctly without modifications across 0.6.x/0.7.x/0.8.x compiler versions
}
```

* [ ] A) Is incorrect and will cause a compilation error
* [x] B) Allows testing with 0.6.11 but accidental deployment with buggy 0.6.5
* [x] C) Is illustrative of risks from using a much older Solidity version (assume current version is 0.8.9)
* [ ] D) None of the above

**Q2 The given contract snippet has**

```solidity
pragma solidity 0.8.4;

contract test {

   // Assume other required functionality is correctly implemented

   function kill() public {
      selfdestruct(payable(0x0));
   }
}
```

* [x] A) Unprotected call to selfdestruct allowing anyone to destroy this contract
* [x] B) Dangerous use of zero address leading to burning of contract balance
* [ ] C) A compiler error because of the use of the kill reserved keyword
* [ ] D) None of the above

**Q3 The given contract snippet has**

```solidity
pragma solidity 0.8.4;

contract test {
    
    // Assume other required functionality is correctly implemented
    
    modifier onlyAdmin() {
        // Assume this is correctly implemented
        _;
    }
    
    function transferFunds(address payable recipient, uint amount) public {
        recipient.transfer(amount);
   }
}
```

* [x] A) Missing use of `onlyAdmin` modifier on transferFunds
* [ ] B) Missing return value check on transfer
* [x] C) Unprotected withdrawal of funds
* [ ] D) None of the above

**Q4 In the given contract snippet**

```solidity
pragma solidity 0.8.4;

contract test {
    
    // Assume other required functionality is correctly implemented
    
    mapping (uint256 => address) addresses;
    bool check;
    
    modifier onlyIf() {
        if (check) {
            _;
        }
    }
    
    function setAddress(uint id, address addr) public {
        addresses[id] = addr;
    }
    
    function getAddress(uint id) public onlyIf returns (address) {
        return addresses[id];
    }
}
```

* [x] A) getAddress returns the expected addresses if check is true
* [x] B) getAddress returns zero address if check is false
* [ ] C) getAddress reverts if check is false
* [ ] D) None of the above

<mark style="color:red;">**Q5 The security concern(s) in the given contract snippet is/are**</mark>

```solidity
pragma solidity 0.8.4;

contract test {
    
    // Assume other required functionality is correctly implemented
    
    modifier onlyAdmin {
     // Assume this is correctly implemented
        _;
    }
    
    function delegate (address addr) external {
        addr.delegatecall(abi.encodeWithSignature("setDelay(uint256)"));
    }
}
```

* [x] A) Potential controlled delegatecall risk -> `addr` is controlled by user
* [x] B) delegatecall return value is not checked
* [x] C) `delegate()` may be missing `onlyAdmin` modifier
* [x] D) `delegate()` does not check for contract existence at addr -> Still returns `true` if `addr` is non-existent

**Comment:**

> Controlled delegatecall: delegatecall() or callcode() to an address controlled by the user allows execution of malicious contracts in the context of the caller’s state. Ensure trusted destination addresses for such calls.

from point 12 of [Security Pitfalls & Best Practices 101 - by Secureum](https://secureum.substack.com/p/security-pitfalls-and-best-practices-101)

> Return values of low-level calls: Ensure that return values of low-level calls (call/callcode/delegatecall/send/etc.) are checked to avoid unexpected failures.

from point 37 of [Security Pitfalls & Best Practices 101 - by Secureum](https://secureum.substack.com/p/security-pitfalls-and-best-practices-101)

> Incorrect access control: Contract functions executing critical logic should have appropriate access control enforced via address checks (e.g. owner, controller etc.) typically in modifiers. Missing checks allow attackers to control critical logic.

from point 4 of [Security Pitfalls & Best Practices 101 - by Secureum](https://secureum.substack.com/p/security-pitfalls-and-best-practices-101)

> Account existence check for low-level calls: Low-level calls call/delegatecall/staticcall return true even if the account called is non-existent (per EVM design). Account existence must be checked prior to calling if needed.

from point 38 of [Security Pitfalls & Best Practices 101 - by Secureum](https://secureum.substack.com/p/security-pitfalls-and-best-practices-101)

**Q6 The vulnerability/vulnerabilities present in the given contract snippet is/are**

```solidity
pragma solidity 0.7.0;
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
// which works with 0.7.0

contract test {

 // Assume other required functionality is correctly implemented
 // For e.g. users have deposited balances in the contract
 // Assume nonReentrant modifier is always applied
  
    mapping (address => uint256) balances;
    
    function withdraw(uint256 amount) external nonReentrant {
        msg.sender.call{value: amount}("");
        balances[msg.sender] -= amount;
    }
}
```

* [ ] A) Reentrancy -> ReentrancyGuard is not inherited
* [ ] B) Integer underflow leading to wrapping -> `balances[msg.sender] -= amount`
* [ ] C) Missing check on user balance in `withdraw()` -> Should check if `balances[msg.sender] >= amount`
* [x] D) All of the above

**Q7 The security concern(s) in the given contract snippet is/are**

```solidity
pragma solidity 0.8.4;

contract test {

 // Assume other required functionality is correctly implemented
    
    uint256 private constant secret = 123;
    
    function diceRoll() external view returns (uint256) {
        return (((block.timestamp * secret) % 6) + 1);
    }
}
```

* [ ] A) `diceRoll()` visibility should be public instead of external
* [x] B) The private variable secret is not really hidden from users
* [x] C) `block.timestamp` is an insecure source of randomness
* [ ] D) Integer overflow

**Q8 The security concern(s) in the given contract snippet is/are**

```solidity
pragma solidity 0.8.4;

contract test {
    
    // Assume other required functionality is correctly implemented
    // Contract admin set to deployer in constructor (not shown)
    address admin;
    
    modifier onlyAdmin {
        require(tx.origin == admin);
        _;
    }
    
    function emergencyWithdraw() external payable onlyAdmin {
        msg.sender.transfer(address(this).balance);
    }
}
```

* [ ] A) Incorrect use of `transfer()` instead of using `send()`
* [x] B) Potential man-in-the-middle attack on admin address authentication
* [ ] C) Assumption on contract balance might cause a revert
* [x] D) Missing event for critical `emergencyWithdraw()` function

**Q9 The given contract snippet is vulnerable because of**

```solidity
pragma solidity 0.8.4;

contract test {
    
    // Assume other required functionality is correctly implemented

    uint256 private constant MAX_FUND_RAISE = 100 ether;
    mapping (address => uint256) contributions;
  
    function contribute() external payable {
        require(address(this).balance != MAX_FUND_RAISE);
        contributions[msg.sender] += msg.value;
    }
}
```

* [ ] A) Integer overflow leading to wrapping
* [ ] B) Overly permissive function visibility of `contribute()`
* [ ] C) Incorrect use of `msg.sender`
* [x] D) Use of strict equality (`!=`) may break the `MAX_FUND_RAISE` constraint

**Q10 In the given contract snippet, the require check will**

```solidity
pragma solidity 0.8.4;

contract test {
    
    // Assume other required functionality is correctly implemented
    
    function callMe (address target) external {
        (bool success, ) = target.call("");
        require(success);
    }
}
```

* [ ] A) Pass only if target is an existing contract address
* [x] B) Pass for a non-existent contract address
* [ ] C) Pass always
* [ ] D) Fail always

**Q11 The security concern(s) in the given contract snippet is/are**

```solidity
pragma solidity 0.8.4;

contract test {
    
    // Assume other required functionality is correctly implemented
    // Assume admin is set correctly to contract deployer in constructor
    address admin;
    
    function setAdmin (address _newAdmin) external {
        admin = _newAdmin;
    }
}
```

* [x] A) Missing access control on critical function
* [x] B) Missing zero-address validation
* [x] C) Single-step change of critical address
* [x] D) Missing event for critical function

**Q12 The security concern(s) in the given contract snippet is/are**

```solidity
pragma solidity 0.8.4;

contract test {

    // Assume other required functionality is correctly implemented 

    address admin;
    address payable pool;

   constructor(address _admin) {
        admin = _admin;
    }    

    modifier onlyAdmin {
        require(msg.sender == admin);
        _;
    }
    
    function setPoolAddress(address payable _pool) external onlyAdmin {
        pool = _pool;
    }

    function addLiquidity() payable external {
        pool.transfer(msg.value);
    }
}
```

* [x] A) Uninitialized pool storage variable which assumes `setPoolAddress()` will be called before addLiquidity()
* [ ] B) Incorrect use of modifier onlyAdmin on `setPoolAddress()`
* [x] C) Missing zero-address validation for \_pool in `setPoolAddress()`
* [x] D) Transaction order dependence risk from admin front-running with pool address change

***

<mark style="color:red;">**Q13 The security concern(s) in the given proxy-based implementation contract snippet is/are**</mark>

```solidity
pragma solidity 0.8.4;
import "https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/proxy/utils/Initializable.sol";

contract test is Initializable {
    
    // Assume other required functionality is correctly implemented
    
    address admin;
    uint256 rewards = 10;
    
    modifier onlyAdmin {
        require(msg.sender == admin);
        _;
    }
    
    function initialize (address _admin) external {
        require(_admin != address(0));
        admin = _admin;
    }
    
    function setRewards(uint256 _rewards) external onlyAdmin {
        rewards = _rewards;
    }
}
```

* [ ] A) Imported contracts are not upgradeable -> Initializable does not have to be upgradeable
* [x] B) Multiple `initialize()` calls possible which allows anyone to reset the admin
* [x] C) rewards will be 0 in the proxy contract before `setRewards()` is called by it -> `uint256 rewards = 10` is set in the implementation contract but not in the proxy contract
* [ ] D) All the above

**Comment:**

There are no imported contracts that need to be made upgradable (by implementing Initializable).

> Import upgradeable contracts in proxy-based upgradeable contracts: Contracts imported from proxy-based upgradeable contracts should also be upgradeable where such contracts have been modified to use initializers instead of constructors.

from point 97 of [Security Pitfalls & Best Practices 101 - by Secureum](https://secureum.substack.com/p/security-pitfalls-and-best-practices-101)

> Unprotected initializers in proxy-based upgradeable contracts: Proxy-based upgradeable contracts need to use public initializer functions instead of constructors that need to be explicitly called only once. Preventing multiple invocations of such initializer functions (e.g. via initializer modifier from OpenZeppelin’s Initializable library) is a must.

from point 95 of [Security Pitfalls & Best Practices 101 - by Secureum](https://secureum.substack.com/p/security-pitfalls-and-best-practices-101)

> Initializing state-variables in proxy-based upgradeable contracts: This should be done in initializer functions and not as part of the state variable declarations in which case they won’t be set.

from point 96 of [Security Pitfalls & Best Practices 101 - by Secureum](https://secureum.substack.com/p/security-pitfalls-and-best-practices-101)

<mark style="color:red;">**Q14 The security concern(s) in the given contract snippet is/are**</mark>

```solidity
pragma solidity 0.8.4;

import "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol";

contract test {
    
    // Assume other required functionality is correctly implemented
    
    address admin;
    address token;

    constructor(address _admin, address _token) {
        require(_admin != address(0));
        require(_token != address(0));
        admin = _admin;
        token = _token;
    }
    
    modifier onlyAdmin {
        require(msg.sender == admin);
        _;
    }
    
    function payRewards(address[] calldata recipients, uint256[] calldata amounts) external onlyAdmin {
        for (uint i; i < recipients.length; i++) {
            IERC20(token).transfer(recipients[i], amounts[i]);
        }
    }
}
```

* [x] A) Potential out-of-gas exceptions due to unbounded external calls within loop
* [ ] B) ERC20 `approve()` race condition
* [x] C) Unchecked return value of `transfer()` (assuming it returns a boolean/other value and does not revert on failure) -> Note that this is ERC20 `transfer()`, not ETH `transfer()`. ERC20 `transfer()` does have a return value.
* [x] D) Potential reverts due to mismatched lengths of recipients and amounts arrays

**Comment:**

There's no guarantee that the passed arrays are of same length, so if one would be longer than the other one it can cause an Out Of Bounds error, which is why D is correct.

> Calls inside a loop: Calls to external contracts inside a loop are dangerous (especially if the loop index can be user-controlled) because it could lead to DoS if one of the calls reverts or execution runs out of gas. Avoid calls within loops, check that loop index cannot be user-controlled or is bounded.

from point 43 of [Security Pitfalls & Best Practices 101 - by Secureum](https://secureum.substack.com/p/security-pitfalls-and-best-practices-101)

> ERC20 transfer() does not return boolean: Contracts compiled with solc >= 0.4.22 interacting with such functions will revert. Use OpenZeppelin’s SafeERC20 wrappers.

from point 24 of [Security Pitfalls & Best Practices 101 - by Secureum](https://secureum.substack.com/p/security-pitfalls-and-best-practices-101)

> This is ERC20 token transfer and not Ether transfer (which throws on failure). [ERC20 transfer is typically expected to return a boolean](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/342265d29066e98c5dc88900d001cab3d0c4f0c5/contracts/token/ERC20/ERC20.sol#L113) but non-ERC20-conforming tokens may return nothing or even revert which is typically why [SafeERC20](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/342265d29066e98c5dc88900d001cab3d0c4f0c5/contracts/token/ERC20/utils/SafeERC20.sol) is recommended.

from Rajeev on [Secureum Discord Server](https://discord.com/channels/814328279468474419)

**Q15 The vulnerability/vulnerabilities present in the given contract snippet is/are**

```solidity
pragma solidity 0.8.4;
import "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol";

contract test {

 // Assume other required functionality is correctly implemented
 // For e.g. users have deposited balances in the contract
   
    mapping (address => uint256) balances;
    
    function withdrawBalance() external {
        msg.sender.call{value: balances[msg.sender]}("");
        balances[msg.sender] = 0;
    }
}
```

* [x] A) Reentrancy
* [ ] B) Integer overflow leading to wrapping
* [ ] C) Integer underflow leading to wrapping
* [ ] D) None of the above

<mark style="color:red;">**Q16 The security concern(s) in the given contract snippet is/are**</mark>

```solidity
pragma solidity 0.8.4;

contract test {
// Assume other required functionality is correctly implemented
// Assume that hash is the hash of a message computed elsewhere in contract
// Assume that the contract does not make use of chainID or nonces in its logic

function verify(address signer, bytes32 memory hash, bytes32 sigR, bytes32 sigS, uint8 sigV) internal view returns (bool) {
   return signer == ecrecover(hash, sigV, sigR, sigS);
}
```

* [x] A) Signature malleability risk of ecrecover
* [x] B) Missing use of nonce in message hash may allow replay attacks across transactions
* [x] C) Missing use of chainID in message hash may allow replay attacks across chains
* [x] D) Missing zero-address check for ecrecover return value may allow invalid signatures

**Comment:**

> Signature malleability: The ecrecover function is susceptible to signature malleability which could lead to replay attacks. Consider using OpenZeppelin’s ECDSA library.

from point 23 of [Security Pitfalls & Best Practices 101 - by Secureum](https://secureum.substack.com/p/security-pitfalls-and-best-practices-101)

> Insufficient Signature Information: The vulnerability occurs when a digital signature is valid for multiple transactions, which can happen when one sender (say Alice) sends money to multiple recipients through a proxy contract (instead of initiating multiple transactions). In the proxy contract mechanism, Alice can send a digitally signed message off-chain (e.g., via email) to the recipients, similar to writing personal checks in the real world, to let the recipients withdraw money from the proxy contract via transactions. To assure that Alice does approve a certain payment, the proxy contract verifies the validity of the digital signature in question. However, if the signature does not give the due information (e.g., nonce, proxy contract address), then a malicious recipient can replay the message multiple times to withdraw extra payments. This vulnerability was first exploited in a replay attack against smart contracts \[36]. This vulnerability can be prevented by incorporating the due information in each message, such as a nonce value and timestamps

from point 3.1.13 of [A Survey on Ethereum Systems Security: Vulnerabilities, Attacks, and Defenses](https://dl.acm.org/doi/fullHtml/10.1145/3391195)

> Indistinguishable Chains: This vulnerability was first observed from the cross-chain replay attack when Ethereum was divided into two chains, namely, ETH and ETC \[10]. Recall that Ethereum uses ECDSA to sign transactions. Prior to the hard fork for EIP-155 \[7], each transaction consisted of six fields (i.e., nonce, recipient, value, input, gasPrice, and gasLimit). However, the digital signatures were not chain-specific, because no chain-specific information was even known back then. As a consequence, a transaction created for one chain can be reused for another chain. This vulnerability has been eliminated by incorporating chainID into the fields.

from point 3.2.1 of [A Survey on Ethereum Systems Security: Vulnerabilities, Attacks, and Defenses](https://dl.acm.org/doi/fullHtml/10.1145/3391195)

> `ecrecover() returns (address)`: recover the address associated with the public key from elliptic curve signature or return zero on error.

from of [Mathematical and Cryptographic Functions – Solidity Documentation](https://docs.soliditylang.org/en/v0.8.0/units-and-global-variables.html#mathematical-and-cryptographic-functions)