Yieldy contest - Chom's results

Lines of code

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

Vulnerability details

Impact

Cannot mint to exactly max supply using _mint function

Proof of Concept

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

if _totalSupply == MAX_SUPPLY this assert will be failed and reverted.

But is shouldn't be reverted as _totalSupply == MAX_SUPPLY is valid.

Tools Used

Manual

Recommended Mitigation Steps

Change to

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

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/README.md?plain=1#L92

Vulnerability details

Impact

Timelock should be implemented to prevent malicious DAO members from rug pull all money. Without Timelock, DAO members may become devil at anytime and formed the gang to take over the DAO multisig and steal the fund at anytime if the fund is big enough to let them retire instantly. Or maybe some incident that allow hackers to hold majority of private key involved in the multisig.

Proof of Concept

If majority of private keys that are used to control multisig are in control of devil people that want to rug pull all money, they can upgrade all contract to the malicious one instantly without passing traditional DAO process (that takes weeks). They can rug pull all money from users in 1 block, nobody even experienced hacker may have a chance to withdraw their fund.

Majority of private keys can be in control of devil people in these case but not limited to

1. Majority of DAO members discussed privately that they will rug pull all money.
2. Private keys are stolen similar to Ronin bridge case.

Tools Used

Manual

Recommended Mitigation Steps

You should clone Timelock contract from a well known project. And set the owner of all upgradable contracts to Timelock. Finally, set owner of Timelock contract to DAO multisig. DAO multisig should interact with Timelock to delay their action. Timelock delay must be at least 1 day to leave enough time for all user to withdraw their fund. If malicious DAO members trying to rug pull all money, users will be alerted and panic to withdraw their fund. So, rug pull become pointless now.

https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol

USE A MORE RECENT VERSION OF SOLIDITY

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

Consider using custom errors instead of revert strings

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deployment cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Any require statement in your code can be replaced with custom error for example,

        // verify that we have enough stakingTokens
        require(
            IERC20Upgradeable(stakingToken).balanceOf(address(this)) >=
                amountToWithdraw,
            "Not enough funds"
        );

Can be replaced with

// declare error before contract declaration
error NotEnoughFunds();

if (IERC20Upgradeable(stakingToken).balanceOf(address(this)) < amountToWithdraw) revert NotEnoughFunds();