Yieldy contest - minhquanym's results

Lines of code

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

Vulnerability details

Impact

In Staking contract, users are allowed to stake for another _recipient. And it also updates warmUpInfo.expiry for _recipient address so _recipient can only claim after this new expiry is passed.

Attackers can abuse this to constantly stake 1 wei for another users, keep increasing expiry value and cause users unable to call claim()

Proof of Concept

Scenario

1. Alice stakes 10000 STAKING_TOKEN into Staking
2. Bob (attacker) constantly call stake(1, address(Alice)) right before expiry is passed.
3. Alice never able to call claim() because this check always failed

Tools Used

Manual Review

Recommneded Mitigation Steps

Consider remove this feature or add an option for users to allow others stake for themselves.

Lines of code

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

Vulnerability details

Impact

In BatchRequests.removeAddress() function, it uses delete keyword to remove contracts[i] from contracts array. This actually only set contracts[i] = address(0) and not do anything with the length of contracts array. The result is elements of contracts array is removed but the length of array is still the same (should decrease instead).

It can lead to the case when contracts array is too large but almost full of address(0). And because sendWithdrawalRequests() is processed the whole array each time, it can lead to out of gas.

Proof of Concept

Please refer to Solidity docs https://docs.soliditylang.org/en/develop/types.html#delete

Tools Used

Manual Review

Recommended Mitigation Steps

We can delete an element of array and update the length of array by swapping the element to the end of array and pop it.

Lines of code

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

Vulnerability details

Impact

In LiquidityReserve, instantUnstake() will try to claimWithdraw() to get more funds before transfering to recipient. But in Staking contract, instantUnstakeReserve() only checks if current balance of reserve is enough (not accounted for unclaimed amount).

This can lead to the case when users should be able to instantUnstake() after claimWithdraw() and balance of reserve is increased. But because it fails this check then the TX is failed.

Proof of Concept

Scenario

1. Current balance of reserve is 1000 and cooldown amount is 500 (and ready to claim)
2. Users call instantUnstakeReserve() with amount = 1100. It should not fail because totalValue of reserve is now 1500.
3. But because it check current balance of reserve which is 1000 < 1100, the TX is failed.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider remove this check because it will fail if reserve don’t have enough funds anyway or call claimWithdraw() before check reserve balance.

Lines of code

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

Vulnerability details

Impact

In Yieldy contract, data about rebase is stored in rebases list everytime rebase() is called and then emits event. It does that by calling _storeRebase() and pass in _previousCirculating and push in rebases data like this

totalStakedBefore: _previousCirculating,
totalStakedAfter: _totalSupply,

But because it uses updatedTotalSupply as _previousCirculating and _totalSupply is updated in this line, totalStakedBefore and totalStakedAfter is basically the same value.

This bug will have impact on client side (Frontend, Backend) because data emitted is incorrect.

Proof of Concept

rebase() function:

_totalSupply = updatedTotalSupply;

_storeRebase(updatedTotalSupply, _profit, _epoch);

and in _storeRebase() function

totalStakedBefore: _previousCirculating,
totalStakedAfter: _totalSupply,

Tools Used

Manual Review

Recommended Mitigation Steps

Use correct variable to pass in _storeRebase() function

Lines of code

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

Vulnerability details

Impact

In Yieldy._mint() function, total supply of Yieldy is capped at MAX_SUPPLY. But the logic is wrong, making the total supply of Yieldy cannot reach MAX_SUPPLY (only maximum MAX_SUPPLY - 1)

require(_totalSupply < MAX_SUPPLY, "Max supply");

This will prevent Staking contract from minting Yieldy token exactly. And also it is inconsistent with rebase() function which allow to update total supply to maximum MAX_SUPPLY.

if (updatedTotalSupply > MAX_SUPPLY) {
    updatedTotalSupply = MAX_SUPPLY;
}

Proof of concept

1. Yieldy has MAX_SUPPLY = 10M, current _totalSupply = 7M.
2. Staking contract call mint() to mint _amount = 3M more Yieldy. But mint() will revert because total supply after add new amount not smaller than MAX_SUPPLY.

_totalSupply + _amount = 10M = maxSupply

Tools Used

Manual Review

Recommended Mitigation Steps

Update < to <=

require(_totalSupply <= MAX_SUPPLY, "Max supply");

1. Save gas by using memory variable instead of storage ones.

In LiquidityReserve.enableLiquidityReserve(), storage variable stakingContract is assigned using _stakingContract so basically they have same value. We should use _stakingContract to save gas because it’s memory variable.

Affected Codes

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

2. Save gas by loading warmUpInfo to memory only after making sure we will use it.

In Staking.claim() function, Claim data is loaded to memory in the beginning of function even when if condition can be failed. Should put it inside if block to save gas like this

if (_isClaimAvailable(_recipient)) {
	Claim memory info = warmUpInfo[_recipient];
	// do something
}

Affected Codes

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