Yieldy contest - pashov's results

Lines of code

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

Vulnerability details

Proof of Concept: the stake function can be called for a different _recipient than msg.sender. Each time you stake tokens your warmUpInfo.expiry or the time when you are able to claim rewards grows by warmUpPeriod as is visible the last line here (not the line with the closing braces):

warmUpInfo[_recipient] = Claim({
       amount: info.amount + _amount,
       credits: info.credits +
       IYieldy(YIELDY_TOKEN).creditsForTokenBalance(_amount),
       expiry: epoch.number + warmUpPeriod
});

This means that a bad actor can always call the stake function with an amount 1 wei of the staking token for a _recipient that the bad actor wants to attack via DoS. he can basically stake a few minutes before the expiry period ends which will again add warmUpPeriod seconds that the staker has to wait until rewards can be claimed. This can be done for a very long time just by paying gas and staking 1 wei of staking token.

Impact: a perfectly good user of the protocol might be hit with a DoS attack and claiming rewards from the contract won’t be available for him

Recommended Mitigation Steps: replace _recipient parameter with a msg.sender usage in the code. This way only a given staker can stake tokens for himself, so no one else can push his expiry time further away in time

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Yieldy.sol#L205

Vulnerability details

Proof of Concept: transferFrom method does not check if _to argument is the zero address

Impact: this can lead to token burns without calling the burn function, which has access control onlyRole(MINTER_BURNER_ROLE) but here this can be bypassed by passing the zero address as the value of _to

Recommended Mitigation Steps: add a non-zero address check for _to argument in transferFrom

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L674 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L485

Vulnerability details

Proof of Concept: the unstake and claimWithdraw methods in Staking.sol are not following the checks-effects-interactions pattern which can lead to reentrancy bugs. The contract does external calls and changes state after the calls.

Impact: if a reentrancy attack on unstaking/withdrawing is successfully executed this can drain the funds out of the smart contract

Recommended Mitigation Steps: always follow the checks-effects-interactions pattern and always change state before external calls. Another option is to add the OpenZeppelin’s ‘nonReentrant’ modifier to the vulnerable functions

Use Solidity Custom Errors instead of require + string statements

Explanation: Solidity 0.8.x adds Custom Errors which are a lot more gas efficient than require statements with strings. Using them also helps with other smart contracts integrating with the codebase

Recommendation: replace all require statements with Custom Errors

Using x = x + y is more gas efficient than x+=y, same for x = x - y and x-=y

Scope: Staking#310, Staking#309, Staking#694, Staking#494, Staking#538

Explanation: the code amountLeft -= warmUpBalance; has worse gas efficiency than the code amountLeft = amountLeft - warmUpBalance; and it is the same for the addition.

Recommendation: expand the expressions in scope as in the given example

In solidity ++var; is more gas efficient than var++;

Scope: https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L708

Explanation: The code does epoch.number++; while ++epoch.number; would save gas

Recommendation: Change the code to ++epoch.number;

COW_SETTLEMENT and COW_RELAYER can be constants

Scope: https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L73-L74

Explanation: The COW_SETTLEMENT and COW_RELAYER variables are always set to the same addresses in the initialize method so they can be turned into constants for gas savings

Recommendation: Change COW_SETTLEMENT and COW_RELAYER to be constants in StakingStorage.sol and hardcode those addresses there

Variable packing can be used in Epoch struct

Scope: https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/structs/Epoch.sol#L4

Explanation: The Epoch struct uses only uint256 as the type for it’s members but most of them are either time-based or counters. Changing duration, number, timestamp and endTime to type uint128 will save a lot of gas when reading an Epoch object.

Recommendation: Use variable packing by changing duration, number, timestamp and endTime to type uint128