Yieldy contest - Picodes's results

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L126 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L176 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L206

Vulnerability details

Impact

Using sandwich attacks and JIT (Just-in-time liquidity), the yield of LiquidityReserve could be extracted for liquidity providers.

Proof of Concept

The yield of LiquidityReserve is distributed when a user calls instantUnstakeReserve() in Staking. Then, in instantUnstake, totalLockedValue increases with the fee paid by the user withdrawing. The fee is shared between all liquidity providers as they all see the value of their shares increase.

Therefore, an attacker could do the following sandwich attack when spotting a call to instantUnstakeReserve().

• In a first tx before the user call, borrow a lot of stakingToken and addLiquidity
• The user call to instantUnstakeReserve()leading to a fee of sayx`
• In a second tx after the user call, removeLiquidity and repay the loan, taking a large proportion of the user fee

The problem here is that you can instantly add and remove liquidity without penalty, and that the yield is instantly distributed.

Recommended Mitigation Steps

To mitigate this, you can

• store the earned fees and distribute them across multiple blocks to make sure the attack wouldn’t be worth it
• add a small fee when removing liquidity, which would make the attack unprofitable
• prevent users from withdrawing before X blocks or add a locking mechanism

3 Non Critical - 4 Low

Overall the code and documentations are of high standard.

[NC - 01] - Incorrect variable names

Here and here, the new value is called _vestingPeriod in both functions, which is confusing and looks like a typo.

[NC - 02] - Use _getTokemakBalance when possible for readability

Here, you re do _getTokemakBalance, so it’d be cleaner to use the internal function. Furthermore line 372 you do a useless casting ITokePool(tokePoolContract) as tokePoolContract is already of type ITokePool.

[NC - 03] - Incorrect comment

Here, the comment states: “the only way balance < _amount is when using unstakeAllFromTokemak”, but this in false:

In unstakeAllFromTokemak, due to this line, you’d have balance = amount.

[Low - 01] - Missing storage gaps for upgradeable contracts might lead to storage slot collision

For upgradeable contracts, there must be a storage gap to "allow developers to freely add new state variables in the future without compromising the storage compatibility with existing deployments" (quote OpenZeppelin). Otherwise, it may be very difficult to write new implementation code. Without storage gaps, the variables in child contracts might be overwritten by the upgraded base contracts if new variables are added to the base contract.

This is a development best practice, and not a large issue in the case of this audit as the contracts do not inherit one another.

(Credits to code-423n4/2022-05-alchemix-findings#44)

Mitigation Steps:

Recommend adding appropriate storage gaps at the end of the storage upgradeable contracts. Please reference OpenZeppelin upgradeable contract templates.

uint256[50] private __gap;

Code: https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/StakingStorage.sol#L7 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserveStorage.sol#L4 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/YieldyStorage.sol#L6

[Low - 02] - Make sure to also initialize implementations when using upgradeable contracts

When using an upgradeable proxy over an implementation it is important to ensure the underlying implementation is initialised in addition to the proxied contract.

Consider the following example where we have a TransparentUpgradeableProxy as contractA and a Implementation as contractB . As a parameter to the constructor of contractA the data is provided to make a delegate call to contractB.initialize() which updates the storage as required in contractA .

Thus, contractB has not been initialized, only contractA has had its state updated. As a result any user is able to call initialize() on contractB , giving themselves full permissions over the contract. There are two reasons why it is not desirable for malicious users to have full control over a contract:

1. Any delegate calls to external contracts can call selfdestruct which would delete the implementation contract temporarily making the proxy unusable (until it can be updated with a new implementation); This is not the case for your implementations from what I’ve seen (otherwise this would have been a high issue).
2. Scammers and malicious users may use implementation contracts, and you’d rather avoid this and keep control over them..

Recommendations

Ensure that the initialize() function is called on all underlying implementations during deployment. This could easily be done by adding in the implementation:

// @custom:oz-upgrades-unsafe-allow constructor
constructor() initializer {}

[Low - 03] - Consider Implementing ERC4626 for LiquidityReserve

Reference: https://eips.ethereum.org/EIPS/eip-4626

LiquidityReserve could theoretically be an ERC4626. To facilitate eventual listings, integrations and increase composability, consider respecting the interface as it isn’t far from what you already have. I feel this could even be a medium issue as EIP are what makes DeFi composable, so should be followed when possible.

This also holds for Yieldy tokens, although it would require more work. The best solution in this case could be to have an ERC4626 wrapper.

But again, I expect this standard to lead to the creation of aggregators or standardize UX. Also it’ll be way more easier for all smart contract based funds managers (ex Yearn) to use your contracts and benefit from your strategy if you respect the standard.

[Low - 04] - Improve checks during deployment.

The safety of deployment could be increased: for example it could be checked in Staking or LiquidityReserves that the stakingToken are the same to avoid unusable deployments.

[Gas - 01] - Duplicate function

In BatchRequests, contracts being public, there is no need for getAddressByIndex.

Code: https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L65 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L9

[Gas - 02] - When removing an entry from an array, place it at the end of the array before deleting

In BatchRequests, considering the frequencies of iterations of the contracts array, you’ll save gas if before removing an entry in contracts, you place it at the end of the array.

This way you can reduce the length of contracts by 1, and you save gas and SLOAD on each iteration.

Instead of delete contracts[i]; you can do:

contracts[i] = contracts[contracts.length - 1];
contracts. pop();