Y2k Finance contest - joestakey's results

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/lib/openzeppelin-contracts/contracts/token/ERC1155/ERC1155.sol#L405 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L217

Vulnerability details

In the Readme, it is mentioned a receiver can call withdraw on behalf of the shares owner

we accept deposits and withdraws on behalf of other users, by using approve ERC1155 functions on withdraw, and recipient/owner params inside both deposit/withdraw functions.

This is done with a check in withdraw using ERC1155.isApprovedForAll(owner, receiver).

The problem is that there is no way to approve a receiver for a specific market: isApprovedForAll(owner, receiver) means the receiver can transfer all the tokens of the owner, regardless of the id - ie regardless of the market in question.

Impact

Medium

Proof Of Concept

• Alice owns 1,000 shares for a market of id M, and 1,000 shares of id N.
• the epoch of market M has ended, and Alice grants approval to Bob to withdraw her shares.
• Bob calls withdraw(M, 1000, Bob, Alice)
• several days later, the epoch of the market of id N ends.
• Bob calls withdraw(N, 10000, Bob, Alice) before Alice.

Bob effectively stole Alice's shares in market N.

Tools Used

Manual Analysis

Mitigation

This is an inherent issue to ERC1155 tokens: approval cannot be granted to a specific id

A potential mitigation could be to override all ERC1155 functions moving tokens (_safeTransferFrom, _mint, _burn and their batch versions), adding the following line:

_operatorApprovals[account][operator] = false

ensuring an owner approves a receiver for one withdrawal only.

I consider this an issue as the readme explicitly states the will to allow withdrawals from approved receivers to make the vaults usable by DAOs. But as highlighted above, it does not work for individual vaults.

QA Report

Table of Contents

Summary

Low

• Lack of validation in createAssets
• use of assert()
• Unbounded loop can result in DoS

Summary

The main concerns are with the lack of validation in createAssets, which can potentially result in the creation of obsolete markets.

Lack of validation in createAssets

Vault.createAssets does not have enough checks for epochBegin, which means it is technically possible for the admin to create an obsolete market.

Impact

Low

Proof Of Concept

Vault.createAssets only checks that epochBegin >= epochEnd.

317: if(epochBegin >= epochEnd)
318:             revert EpochEndMustBeAfterBegin();
319: 
320:         idExists[epochEnd] = true;
321:         idEpochBegin[epochEnd] = epochBegin;

But for a user to be able to call deposit, the modifier epochHasNotStarted must not revert, ie idEpochBegin[id] - timewindow must be greater than the current block.timestamp

87:     modifier epochHasNotStarted(uint256 id) {
88:         if(block.timestamp > idEpochBegin[id] - timewindow)
89:             revert EpochAlreadyStarted();
90:         _;
91:     }
92: 

This means that the following case can happen:

• Admin calls VaultFactory.createNewMarket, with epochBegin == N and epochEnd == N + 100.
• The current block.timestamp is N - 43,200 (ie N - 0.5 days)
• This means idEpochBegin[id] - timewindow == N - 1 days < block.timestamp: the modifier epochHasNotStarted() will always revert for this market. Users cannot call deposit, the market needs to be re-deployed.

Tools Used

Manual Analysis

Mitigation

Add stronger validation in Vault.createAssets:

-317: if(epochBegin >= epochEnd)
+317: if((epochBegin >= epochEnd) || (epochBegin - timewindow <= block.timestamp))
318:             revert EpochEndMustBeAfterBegin();
319: 
320:         idExists[epochEnd] = true;
321:         idEpochBegin[epochEnd] = epochBegin;

You can also consider replacing timewindow with a greater number.

use of assert()

As per Solidity documentation, assert should be avoided, except to check invariants.

It is used here in depositETH, asserting the WETH.transfer call.

Impact

Low

Tools Used

Manual Analysis

Mitigation

+190: require(IWETH(address(asset)).transfer(msg.sender, msg.value));
-190: assert(IWETH(address(asset)).transfer(msg.sender, msg.value));

Unbounded loop can result in DoS

PROBLEM

When epochs.length increases, it results in the gas cost of Vault.getNextEpoch getting higher. Should epochs reach a certain size, the gas cost of the function call might get too high, meaning every call to Vault.getNextEpoch would revert.

Impact

Low

TOOLS USED

Manual Analysis

PROOF-OF-CONCEPT

epochs gets larger everytime createAssets is called upon the creation of a new market:

322: epochs.push(epochEnd)

This result in the call to Vault.getNextEpoch getting more gas consuming.

443: for (uint256 i = 0; i < epochsLength(); i++) {
444:             if (epochs[i] == _epoch) {
445:                 if (i == epochsLength() - 1) {
446:                     return 0;
447:                 }
448:                 return epochs[i + 1];
449:             }
450:         }

MITIGATION

Either keep epochs to a certain size with an upper boundary, or consider using an Enumerable set.