# RACE #17

{% embed url="<https://ventral.digital/posts/2023/5/1/race-17-of-the-secureum-bootcamp-epoch-infinity>" %}
RACE #17
{% endembed %}

<figure><img src="https://3988450783-files.gitbook.io/~/files/v0/b/gitbook-x-prod.appspot.com/o/spaces%2F-MWVjG_njKgBtvmnKaJh%2Fuploads%2FBkCHTGtLqYkSEQCMTyyQ%2Fimage.png?alt=media&#x26;token=093a77e5-6177-4dbc-92ff-e948ed571a13" alt=""><figcaption><p>RACE #17 result</p></figcaption></figure>

{% hint style="info" %}
*Note: All 8 questions in this RACE are based on the below contract. This is the same contract you will see for all the 8 questions in this RACE. The question is below the shown contract.*
{% endhint %}

```solidity
pragma solidity 0.8.19;


import {Pausable} from "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/Pausable.sol";
import {IERC20} from "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol";
import {SafeERC20} from "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol";


interface IWETH is IERC20 {
   function deposit() external payable;
   function withdraw(uint256) external;
}


contract Vault is Pausable {
   using SafeERC20 for IWETH;


   address public immutable controller;
   IWETH public immutable WETH;


   // about 4700 ETH
   uint72 public constant TOTAL_CONTRIBUTION_CAP = type(uint72).max;
   // ALLOWANCE_CAP = 40% of TOTAL_CONTRIBUTION_CAP
   uint256 public immutable ALLOWANCE_CAP = 40 * uint256(TOTAL_CONTRIBUTION_CAP) / 100;
   uint72 public totalContributions;
   mapping (address => uint72) individualContributions;


   uint256 numContributors;
   event ContributorsUpdated(address newContributor, uint256 indexed oldNumContributors, uint256 indexed newNumContributors);


   constructor(address _controller, IWETH _weth) {
       controller = _controller;
       WETH = _weth;
   }


   function deposit(uint72 amount) external payable whenNotPaused {
       if (msg.value > 0) {
           WETH.deposit{value: amount}();
       } else {
           WETH.transferFrom(msg.sender, address(this), amount);
       }
       require((totalContributions += amount) <= TOTAL_CONTRIBUTION_CAP, "cap exceeded");
       if (individualContributions[msg.sender] == 0) emit ContributorsUpdated(msg.sender, numContributors, numContributors++);
       individualContributions[msg.sender] += amount;
   }


   function withdraw(uint72 amount) external whenNotPaused {
       individualContributions[msg.sender] -= amount;
       totalContributions -= amount;
       // unwrap and call
       WETH.withdraw(amount);
       (bool success, ) = payable(address(msg.sender)).call{value: amount}("");
       require(success, "failed to transfer ETH");
   }


   function requestAllowance(uint256 amount) external {
       // ALLOWANCE_CAP is 40% of TOTAL_CAP
       uint256 allowanceCap = ALLOWANCE_CAP;
       uint256 allowance = amount > totalContributions ? allowanceCap : amount;
       WETH.safeApprove(controller, allowance);
   }


   // for unwrapping WETH -> ETH
   receive() external payable {
       require(msg.sender == address(WETH), "only WETH contract");
   }
}
```

## Question 1 :white\_check\_mark:

*deposit()* can revert:

* [x] &#x20;A. If insufficient ETH was sent with the call
* [x] &#x20;B. If the caller has insufficient WETH balance
* [ ] &#x20;C. With the "cap exceeded" error
* [ ] &#x20;D. If called internally

**Comment:**

A: If the ether (*msg.value*) provided with the call to *deposit()* is less than the specified *amount*, the attempt to call *WETH.deposit()* will revert.

B: If no ether was provided with the call and the caller has an insufficient balance of WETH, or given insufficient approval to their WETH balance, the attempt to call *WETH.transferFrom()* will revert.

C: For the "cap exceeded" error to be thrown, `totalContributions + amount > TOTAL_CONTRIBUTION_CAP`. But `TOTAL_CONTRIBUTION_CAP = type(uint72).max` and *totalContributions* is *uint72*. <mark style="color:red;">**So the attempt to add an**</mark><mark style="color:red;">**&#x20;**</mark>*<mark style="color:red;">**amount**</mark>*<mark style="color:red;">**&#x20;**</mark><mark style="color:red;">**to**</mark><mark style="color:red;">**&#x20;**</mark>*<mark style="color:red;">**totalContributions**</mark>*<mark style="color:red;">**&#x20;**</mark><mark style="color:red;">**that would make it larger than**</mark><mark style="color:red;">**&#x20;**</mark>*<mark style="color:red;">**type(uint72).max**</mark>*<mark style="color:red;">**&#x20;**</mark><mark style="color:red;">**would revert from an integer overflow error before the**</mark><mark style="color:red;">**&#x20;**</mark>*<mark style="color:red;">**require()**</mark>*<mark style="color:red;">**&#x20;**</mark><mark style="color:red;">**ever could.**</mark> This makes the *require()* basically redundant and it can be removed.

D: The *deposit()* function is a external. An attempt to call it internally would not revert but never compile in the first place. The contract could still call this function via *this.deposit()* though, which would make the contract CALL itself like it would an external contract.

## Question 2 :white\_check\_mark:

What issues pertain to the *deposit()* function?

* [ ] &#x20;A. Funds can be drained through re-entrancy
* [ ] &#x20;B. Accounting mismatch if user specifies *amount* > *msg.value*
* [x] &#x20;C. Accounting mismatch if user specifies *amount* < *msg.value*
* [ ] &#x20;D. *totalContributionCap* isn't enforced on an individual level

**Comment:**

A: For a re-entrancy an unsafe external call needs to be made. All the external calls being made within the `deposit()` are to the trusted WETH contract which does also not have any sender callbacks/hooks like an ERC777 would have.

B/C: If the specified *amount* was larger than the sent *msg.value*, the function would revert. But on the other hand, if the *amount* was smaller than the actual sent *msg.value* the deposit would only handle the *amount* and the rest of the ether would be locked in the *Vault* contract.

D: The fact that *totalContributionCap* isn't enforced on an individual level does not cause an issue as *totalContributions*'s value would revert before any individual contributor would be able to make deposits beyond the cap.

## Question 3 :white\_check\_mark:

Which of the following is/are true about *withdraw()*?

* [ ] &#x20;A. Funds can be drained through re-entrancy
* [ ] &#x20;B. Funds can be drained due to improper amount accounting in *deposit()*
* [ ] &#x20;C. Assuming a sufficiently high gas limit, the function reverts from the recipient (caller) consuming all forwarded gas
* [x] &#x20;D. May revert with "failed to transfer ETH"

**Comment:**

A: The *withdraw()* function follows the Checks-Effects-Interactions pattern: (Check) Subtracting the amount from the individual's contribution would revert if the integer were to underflow. (Effect) The individual contributor's balance is updated before the value is transferred. (Interactions) The unsafe external call to the *msg.sender* is only made once all Checks and Effects have been applied. Re-entering the contract would not allow draining any funds.

B: It's true that there's improper accounting, but the effect is that funds would be stuck, not drained.

C: As only 63/64 gas is forwarded, the function should have sufficient gas remaining for execution (hence the high gas limit assumption).

D: True, if msg.sender reverts (eg. is a contract that lacks payable / fallback function).

## Question 4 - This one tricky

Which of the following parameters are correctly emitted in the *ContributorsUpdated()* event?

* [x] &#x20;A. *newContributor*
* [ ] &#x20;B. *oldNumContributors*
* [ ] &#x20;C. *newNumContributors*
* [ ] &#x20;D. None of the above.

**Comment:**

A: Option A is understandably ambiguous: if withdrawals were working, then you could have an existing contributor be recognized as a new one. Nevertheless, in its current state, we can take it to emit for only legitimately new contributors.

B: Generally, it's not safe to make assumptions about the evaluation order of expressions in Solidity. How obscure this can be especially for event emissions has been shown in [last year's Underhanded Solidity contest](https://github.com/ethereum/solidity-underhanded-contest/blob/master/2022/submissions_2022/submission9_TynanRichards/SPOILERS.md): <mark style="color:red;">**"The indexed parameters are evaluated first in right-to-left order, then the non-indexed parameters are evaluated left-to-right"**</mark>. Because of this, the general best practice is to avoid nested expressions whenever possible and do them within separate lines of code.

C: One of the most common gas optimizations seen in Code4rena reports is how the prefix increment (++i) is more efficient than postfix (i++). However, most aren't aware of a key difference: <mark style="color:red;">**Prefix increments returns the value BEFORE the increment, postfix returns the value AFTER**</mark>.

## Question 5 :white\_check\_mark:

The vault deployer can pause the following functions:

* [ ] &#x20;A. *deposit()*
* [ ] &#x20;B. *withdraw()*
* [ ] &#x20;C. *requestAllowance()*
* [x] &#x20;D. None of the above -> `_pause()` and `_unpause()` from Pausable are internal functions, which are not exposed in this contract

## Question 6 :white\_check\_mark:

What is the largest possible allowance given to the *controller*?

* [ ] &#x20;A. 40% of *totalContributionCap*
* [ ] &#x20;B. 60% of *totalContributionCap*
* [x] &#x20;C. 100% of *totalContributionCap* -> This happens when `amount == totalContributions`. Here `totalContributions` can go up to `TOTAL_CONTRIBUTION_CAP` as what is implemented in `deposit()`
* [ ] &#x20;D. Unbounded

## Question 7 :white\_check\_mark:

The *requestAllowance()* implementation would have failed after the 1st call for tokens that only allow zero to non-zero allowances. Which of the following mitigations do NOT work?

* [ ] &#x20;A. *safeApprove(0)* followed by *safeApprove(type(uint256).max)*
* [x] &#x20;B. *safeIncreaseAllowance(type(uint256).max)*
* [x] &#x20;C. *safeIncreaseAllowance(0)* followed by *safeIncreaseAllowance(type(uint256).max)*
* [x] &#x20;D. *safeDecreaseAllowance(0)* followed by *safeApprove(type(uint256).max)*

**Comment:**

There are some implementations of ERC20 tokens that require an approval to be reset to 0 before it can be updated to another non-zero value.

The *safeIncreaseAllowance()* / *safeDecreaseAllowance()* functions request the current allowance, adding/subtracting the specified amount and then update it by calling *approve()*. These functions do not set the approval to 0 in between, so for these tokens the function would still fail after the 1st call.

## Question 8

Which of the following gas optimizations are relevant in reducing runtime gas costs for the vault contract?

* [ ] &#x20;A. Changing *ALLOWANCE\_CAP* type from *immutable* to *constant*, ie. `uint256 public constant ALLOWANCE_CAP = 40 * uint256(TOTAL_CONTRIBUTION_CAP) / 100;`
* [x] &#x20;B. Increase number of solc *runs* (assuming default was *200 runs*)
* [x] &#x20;C. Renaming functions so that the most used functions have smaller method IDs
* [ ] &#x20;D. Use unchecked math in *withdraw()*

**Comment:**

A: Changing *ALLOWANCE\_CAP* to be *constant* would actually consume more runtime gas as the expression would then be evaluated on every call, while with *immutable* the expression would be calculated during deployment and then become a static value. (Note that there's no difference between these options anymore when the via-IR compilation pipeline and optimization is used).

<mark style="color:red;">**B: Increasing the optimizer's**</mark><mark style="color:red;">**&#x20;**</mark>*<mark style="color:red;">**runs**</mark>*<mark style="color:red;">**&#x20;**</mark><mark style="color:red;">**configuration will increase the deployment bytecode size but decrease the gas usage at runtime.**</mark>

<mark style="color:red;">**C: The smaller the function's ID the earlier it can be found by the function selector.**</mark>

D: It wouldn't be safe to use unchecked math in the *withdraw()* function as it would potentially allow users to withdraw more than they should be able to. (The wording is intended to imply that the entire function's body would be put into an unchecked block.)