✅RACE #17

RACE #17 Of The Secureum Bootcamp Epoch∞ • Ventral DigitalVentral Digital

RACE #17

RACE #17 result

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.

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 ✅

deposit() can revert:

• A. If insufficient ETH was sent with the call

• B. If the caller has insufficient WETH balance

• C. With the "cap exceeded" error

• 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. So the attempt to add an amount to totalContributions that would make it larger than type(uint72).max would revert from an integer overflow error before the require() ever could. 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 ✅

What issues pertain to the deposit() function?

• A. Funds can be drained through re-entrancy

• B. Accounting mismatch if user specifies amount > msg.value

• C. Accounting mismatch if user specifies amount < msg.value

• 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 ✅

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

• A. Funds can be drained through re-entrancy

• B. Funds can be drained due to improper amount accounting in deposit()

• C. Assuming a sufficiently high gas limit, the function reverts from the recipient (caller) consuming all forwarded gas

• 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?

• A. newContributor

• B. oldNumContributors

• C. newNumContributors

• 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: "The indexed parameters are evaluated first in right-to-left order, then the non-indexed parameters are evaluated left-to-right". 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: Prefix increments returns the value BEFORE the increment, postfix returns the value AFTER.

Question 5 ✅

The vault deployer can pause the following functions:

• A. deposit()

• B. withdraw()

• C. requestAllowance()

• D. None of the above -> _pause() and _unpause() from Pausable are internal functions, which are not exposed in this contract

Question 6 ✅

What is the largest possible allowance given to the controller?

• A. 40% of totalContributionCap

• B. 60% of totalContributionCap

• C. 100% of totalContributionCap -> This happens when amount == totalContributions. Here totalContributions can go up to TOTAL_CONTRIBUTION_CAP as what is implemented in deposit()

• D. Unbounded

Question 7 ✅

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?

• A. safeApprove(0) followed by safeApprove(type(uint256).max)

• B. safeIncreaseAllowance(type(uint256).max)

• C. safeIncreaseAllowance(0) followed by safeIncreaseAllowance(type(uint256).max)

• 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?

• A. Changing ALLOWANCE_CAP type from immutable to constant, ie. uint256 public constant ALLOWANCE_CAP = 40 * uint256(TOTAL_CONTRIBUTION_CAP) / 100;

• B. Increase number of solc runs (assuming default was 200 runs)

• C. Renaming functions so that the most used functions have smaller method IDs

• 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).

B: Increasing the optimizer's runs configuration will increase the deployment bytecode size but decrease the gas usage at runtime.

C: The smaller the function's ID the earlier it can be found by the function selector.

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.)