Y2k Finance contest - Respx's results

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L65-L83 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L302

Vulnerability details

Description

On lines 68/70 of PegOracle.sol, the ratio between price1 and price2 is scaled up by 10**4. It is then potentially scaled up again on line 74. But if the value of decimals10 on line 73 is low, say if priceFeed1.decimals() is 18 then decimals10 would just be 1, then nowPrice could potentially be less than 10**6. As nowPrice is divided by 10**6 when it is returned on line 78, its value will always be zero in these cases.

Impact

This effect would cause all calls to Controller.latestRoundData() to revert because of the test on line 302 of Controller.sol. This would mean that calls to triggerDepeg() and triggerEndEpoch() would revert, making it impossible to resolve any vaults in the protocol. It would not prevent the creation of vaults or depositing into the vaults.

Proof of Concept

This is a contract built in Remix to simply demonstrate that the mathematics is such that, if the two price values are similar and decimals10 is low (10 in this case), then nowPrice / 1000000 will always return as zero.

contract PegTest {

    function pegTest() external view returns (int256,int256) {

        int price1 = 749844396466845646545;
        int price2 = 755341351873846846465;
        int256 nowPrice = 0;

        if (price1 > price2) {
            nowPrice = (price2 * 10000) / price1;
        } else {
            nowPrice = (price1 * 10000) / price2;
        }

        int256 decimals10 = int256(10**(18 - 17));
        nowPrice = nowPrice * decimals10;

        return (nowPrice, nowPrice / 1000000);
    }
}

Tools Used

Manual inspection

Recommended Mitigation Steps

Do not divide by 1000000 on line 78 of PegOracle.sol.

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L217

Vulnerability details

Description

The access tests at the start of Vault.withdraw() test whether the receiver address is an approved operator in the case where msg.sender is not the balance owner. However, there is no test on the identity of msg.sender. This allows any user to call withdraw at the end of an epoch to transfer another address' balance of asset to any address which that address has approved as an operator for the vault shares.

Impact

Theft

Consider a Uniswap-style liquidity pool of the vault token and the asset token. alice, a user, has approved this liquidity pool as an operator to spend her vault tokens. She does this to allow her to send transactions to trade the tokens freely, confident in the belief that such a permission is safe to give (as many max approval spend permissions are given to liquidity pools for ERC20 tokens). Then consider that a malicious user, bob, issues a transaction at the end of the vault epoch with two parts:

1. Call Vault.withdraw() with owner=alice and recipient=the liquidity pool. This transfers alice's asset balance in the vault to the liquidity pool.
2. Call the skim method of the liquidity pool. This is a method in Uniswap style pools which transfers to the caller any tokens in the pool beyond the expected reserves. This transfers alice's asset balance to bob.

Alternatively, if bob is able to get alice to approve a malicious contract under his control as an operator for the vault tokens, he can also carry out this attack. The attack would not be obvious, as a WETH withdrawal function in the attack contract does not seem like a threat for approval of a different token entirely. So the attack contract could easily seem legitimate.

Locking

This is the scenario in the proof of concept test below. Consider any situation in which a user has granted operator permission for the vault tokens to a contract that does not have facilities for withdrawal of the asset token. Alternatively, a lost wallet could have been granted operator privilege. In either case, the asset tokens can be withdrawn to this inaccessible address by any account.

Proof of Concept

This test demonstrates that any user can withdraw another account's balance to an operator address.

diff --git a/test/AssertTest.t.sol b/test/AssertTestPOC.t.sol
index 8eebca6..5f38fba 100644
--- a/test/AssertTest.t.sol
+++ b/test/AssertTestPOC.t.sol
@@ -316,14 +316,29 @@ contract AssertTest is Helper {
 
         uint assets;
 
-        //ALICE hedge WITHDRAW
+        // Two new accounts to ensure no interference from established accounts
+        address alice_lost_wallet = address(6);
+        address alice_worst_enemy = address(7);
+
+        //ALICE accidentally grants operator status to another wallet she controls
         vm.startPrank(alice);
+        vHedge.setApprovalForAll(alice_lost_wallet,true);
+        //Oops, she lost that wallet's key. Well..never mind. It won't be sending any transactions, so there's no real need to reverse this permission.
+
         assets = vHedge.balanceOf(alice,endEpoch);
-        vHedge.withdraw(endEpoch, assets, alice, alice);
+        emit log_named_uint("alice hedge balance",assets);
+        vm.stopPrank();
+
+        // Oh dear. Here comes a user that is malicious to alice and would never have been given access to her funds
+        vm.startPrank(alice_worst_enemy);
+        //alice_worst_enemy withdraws
+        vHedge.withdraw(endEpoch, assets, alice_lost_wallet, alice);
 
+        //alice's funds have been transferred to her lost wallet by a third party, just because it was approved as an operator (but never a recipient).
         assertTrue(vHedge.balanceOf(alice,endEpoch) == NULL_BALANCE);
         uint256 entitledShares = vHedge.beforeWithdraw(endEpoch, assets);
-        assertTrue(entitledShares - vHedge.calculateWithdrawalFeeValue(entitledShares,endEpoch) == ERC20(WETH).balanceOf(alice));
+        assertTrue(entitledShares - vHedge.calculateWithdrawalFeeValue(entitledShares,endEpoch) == ERC20(WETH).balanceOf(alice_lost_wallet));
+        // NOTE alice_lost_wallet could also be a contract that alice approved to spend her tokens
 
         vm.stopPrank();
 

Tools Used

Manual Inspection

Recommended Mitigation Steps

Change line 217 of Vault.sol to isApprovedForAll(owner, msg.sender) == false)

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/rewards/StakingRewards.sol#L217-L220

Vulnerability details

Description

The tests on lines 217-220 of StakingRewards.sol prevent the owner from withdrawing the staked tokens of users. However, there is no protection to prevent the owner from withdrawing any amount of the reward token.

Impact

The reason this issue has been rated as medium risk is because of the code in notifyRewardAmount() that prevents admin (specifically rewardsDistribution) from creating a reward schedule that cannot be paid out and also the tests in setRewardsDuration() which prevent owner from disrupting the duration until a full reward period has finished. From these design choices, it appears that the development team wishes to create protected, reliable reward periods that cannot be disrupted. It would be quite easy for owner to accidentally withdraw too many reward tokens from the contract and render the contract unable to pay out its rewards, thus damaging the protocol. It would also damage the protocol if all rewards were to suddenly disappear by an accidental full withdrawal.

Proof of Concept

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/rewards/StakingRewards.sol#L217-L220

Tools Used

Manual Inspection

Recommended Mitigation Steps

If withdrawing the rewards token in recoverERC20(), add code to prevent the withdrawal of any tokens that will be needed to fulfill the current reward period.

Overview

The code base would benefit greatly from a clear, simple overview of the protocol. After reading the contest description many times, it took me many hours to realise two key points:

• Y2K doesn't actually deal with the tokens that are being hedged/insured. Instead, users deposit WETH and they are making bets with WETH that the assets will or won't depeg.
• The amounts in the hedge/risk vaults do not have to match, so one could be 10x the other, meaning the rewards for each type of depeg will vary according to market confidence.

Perhaps other wardens found these points self evident, but they were the insights I did not notice in the description which caused the ideas to "click" for me.

Contract files should include file header comments that give an overview of the contract, similar to the descriptions currently on the competition page.

Most functions do have comment headers, many of them helpful.

The code would also be clearer if significantly more comments were added within functions. Almost every line of code can usually be clarified, and this makes reading the code a great deal easier.

The first point in the Non Critical section is also particularly worth noting as it runs throughout the code.

---

Non Critical

(code style, clarity, syntax, versioning, off-chain monitoring (events, etc)

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L162 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L336 The use of epochEnd as an ID number which exists alongside the marketplace ID number is needlessly confusing. Frequently, epochEnd is submitted to a function as a parameter and the parameter is called id (I have linked one example above, but there are more). This is very difficult to read. I strongly recommend renaming epochEnd as something like epochEndId or epochId and consistently using that for this value. It would also help reduce ambiguity to consistently name the marketplace ID variables something like marketId.

VaultFactory.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/VaultFactory.sol#L242 Further to the point above, I think this comment implies that the parameter index is actually the value of epochEnd, whereas is should be the market index.

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/VaultFactory.sol#L268 VaultFactory._createEpoch() is missing its comment header

Controller.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L317 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L12 Consider renaming this function getVaultFactory() to getVaultFactoryAddress() simply because its purpose seems to be to return an address, not a VaultFactory like the public getter on line 12.

PegOracle.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L68-L70 Consider writing 10000 as 10_000 for clarity. https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L78 Similarly, consider writing 1000000 as 1_000_000 for clarity.

Vault.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L150 The comment on line 150 should be punctuated, "from its shares", similar to line 180 (ie. remove the apostrophe on 150).

Low

(e.g. assets are not at risk: state handling, function incorrect as to spec, issues with comments)

VaultFactory.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/VaultFactory.sol#L312 VaultFactory.changeTreasury() modifies both the state variable VaultFactory.treasury but also the treasury value of the specified market index. This has gas implications, submitted separately, but could also cause an administration error. A change to a specific market ID might not be expected to change VaultFactory's treasury state variable. Or a change that updates the state variable might be assumed to update all vaults. Consider separating the function into two: one to change the state variable, the other to change the treasury within specified market vaults. This is the structure used in setController()/changeController().

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/VaultFactory.sol#L184 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/VaultFactory.sol#L221 In createNewMarket, because of the test on line 221, if tokenToOracle[_token] is already set, the value of _oracle will be silently discarded. This could have security implications if the previous oracle is no longer considered reliable. If a new oracle address is provided, it should be updated. Consider changing line 221 to: if (tokenToOracle[_token] != _oracle) { There should also be a require that _oracle is not the zero address.

Controller.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L102 This test on line 102 should probably be vault.idEpochBegin(epochEnd) >= block.timestamp) as users will probably expect a begin time to be inclusive.

Vault.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L110 The comment on line 110 is incorrect: _token is the token address, not the oracle address.

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L265 The comment on line 265 is incorrect. It reads: // 0.5% = multiply by 1000 then divide by 5 It should be: // 0.5% = multiply by 5 then divide by 1000

PegOracle.sol and Controller.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L73 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L299 Both these contracts have expressions of the form 10**(18-(priceFeed.decimals()));. Consider the case where the value of priceFeed.decimals() is greater than 18. This will cause both of these lines to revert, rendering the protocol incompatible with any such price feeds.

VaultFactory.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/VaultFactory.sol#L178-L239 VaultFactory.createNewMarket() accesses many state variables multiple times. Gas can be saved in all cases by copying the state variable into a memory variable and then using the memory variable throughout the code. controller is used on lines 188,192,206,216 treasury is used on lines 203,213 marketIndex is used on lines 195,219,226 (The first used of marketIndex modifies its value, but it can still be used to copy the new value into memory: _marketIndex = marketIndex++;

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/VaultFactory.sol#L312 VaultFactory.changeTreasury() modifies both the state variable VaultFactory.treasury but also the treasury value of the specified market index. If the treasury values for multiple markets are to be changed, the state variable treasury will be written to multiple times. Consider separating the function into two: one to change the state variable, the other to change the treasury within specified market vaults. This is the structure used in setController()/changeController().

Controller.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L97-L99 Consider avoiding the second call to getLatestPrice(vault.tokenInsured() by saving its return value to a memory variable.

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L83-L110 In the modifier isDisaster(), gas can be saved by placing tests first which use the least gas and are the most likely to fail. This is a judgement call best made by the development team, but it may be the case that the last two tests are more likely to fail as they relate to human error. They are also relatively cheap to test. The third test also relates to human error, but is quite expensive. The first two tests need to remain early for logical reasons. Suggested order: 5,1,2,4,3

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L277 There is no need to create this isSequencerUp variable. Save gas with: if (answer != 0) { or if (answer == 1) { as line 278 and removing line 277.

StakingRewards.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/rewards/StakingRewards.sol#L109-L112 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/rewards/StakingRewards.sol#L114 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/rewards/StakingRewards.sol#L132 exit() calls the modifiers nonReentrant updateReward(msg.sender) twice. This burns extra gas for many users who are likely to use this function. Gas can be saved by creating internal functions with the functionality of withdraw() and getReward(). The external functions would then look something like this:

function exit() external nonReentrant updateReward(msg.sender) {
	_withdraw(_balances[msg.sender]);
	_getReward();
}
function withdraw(uint256 amount) external nonReentrant updateReward(msg.sender) {
	_withdraw(amount);
}
function getReward(uint256 amount) external nonReentrant updateReward(msg.sender) {
	_getReward(amount);
}

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/rewards/StakingRewards.sol#L61 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/rewards/StakingRewards.sol#L176 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/rewards/StakingRewards.sol#L159-L171 updateReward() calls earnings and between them these functions call rewardPerToken() twice. Unfortunately rewardPerToken() is relatively a expensive function for retrieving a value as it involves some calculation. updateReward() is also a modifier that will be called frequently, so the impact of this issue could be quite high. Consider writing out the logic of updateReward() to avoid the call to earnings() so that the return value of the first call to rewardPerToken() can be reused.

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/rewards/StakingRewards.sol#L183-L210 notifyRewardAmount() accesses the state variable rewardsDuration multiple times. Gas can be saved by copying the state variable into a memory variable and then using the memory variable throughout the code. rewardsDuration is used on lines 190,194,203,208.

PegOracle.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L73 The call to priceFeed1.decimals() is unnecessary as this value has already been stored in the state variable decimals. The only reason to continue making the external call would be if the price feed's decimals might change, but then the same check would be needed for priceFeed2.decimals(), and that is not being made. Consider rewrting line 73 as: int256 decimals10 = int256(10**(18 - decimals));