Yieldy contest - zzzitron's results

WarmUp expiry can be prolonged by staking from somebody else

• Staking.sol:406
• Staking.sol:439-444
• Staking.sol:691

Impact

When warmUpPeriod is greater than 1, a third person can stake to the victim to prolong the warmUp expiry. The expiry prolongation also happens with cool down, although a third party cannot do the similar attack. If the user unstakes within the cool down period, the cool down expiry will be pushed back.

Proof of Concept

This proof of concept demonstrates that staker2 is staking for staker1, and as the result staker1's warm up expiry is updated. The warmUpPeriod in the above example was set to 2. The staker1 stakes and waits for one epoch, then staker2 stakes a small amount on behalf of staker1. After another epoch, the staker1 expects to be able to claim, however since the staker2 staked in the previous epoch, the staker1 cannot claim. If the staker2 keep using this tactic, the staker1 cannot claim unless the warmUpPeriod is set to be less than or equal to 1. Otherwise, the staker1 should just unstake.

Tools Used

hardhat

Recommended Mitigation Steps

The stake(uint256,address) is used in the Migration, so it is needed. One idea to prevent another person to call it is, to add an access control. Another idea is to manage the warmUpInfo and coolDownInfo differently.

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L79-L80 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L157-L160

Vulnerability details

Impact

After updating the curve pool using setCurvePool, one cannot use instantUnstakeCurve anymore, because TOKE_POOL is not approved to the new curve pool. There is no other way to approve, so the instantUnstakeCurve functionality cannot be used with any new curve pool. When no curve pool was set in the initialize function, then there is no way to use the instantUnstakeCurve functionality.

Proof of Concept

This proof of concept demonstrates that the instantUnstakeCurve reverts after the curve pool is updated. The set up for the proof of concept is almost identical to the test/stakingTest.ts except the curve pool was set to zero in the initialize. Then the curve pool was updated by calling the setCurvePool function, then attempt to call instantUnstakeCurve, which reverts.

Tools Used

hardhat

Recommended Mitigation Steps

Add the approve logic to the setCurvePool or setToAndFromCurve. Also consider adding logic to un-approve the previous curve pool, just in case the curve pool might be compromised.

Yieldy QA Report

Summary / Impression

• The code is well structured, however can be cleaner by refactoring. For example, using creditsForAmount instead of manually calculating in Yieldy. It is easy to read and less error-prone to reuse functions..
• More comments on what variables are supposed to do, can help to understand the code.
• The tests helped to understand the codebase and are easy to work with.

Low

L01: transferFrom will send to zero address in Yieldy

• code

In the transferFrom functions, the _to address is not checked for zero address.

L02: missing balance check in Yieldy

• code

        creditBalances[_from] = creditBalances[_from] - creditAmount;

When updating the creditBalances in the transferFrom, the balance was not checked against the transferring amount. A transaction transferring more than the balance will not go through, thanks to the underflow check in solidity version 8. But it is still good to add the check to revert with the proper reason or custom error.

L03: missing zero address check in BatchRequests

• code
• code

Upon removeAddress, it will delete the item at index i. So, to use the contracts array, one should be prepared to encounter zero addresses.

// in canBatchContracts
            bool canBatch = IStaking(contracts[i]).canBatchTransactions();

The canBatchContracts and canBatchContractByIndex do not check for the zero address. It will revert when those functions should make a call to the zero address.

Non-critical

N01: duplicated code in Staking

• code

Staking contract is approving liquidity reserve twice.