Yieldy contest - 0x1f8b's results

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/34774d3f5e9275978621fd20af4fe466d195a88b/src/contracts/BatchRequests.sol#L93 https://github.com/code-423n4/2022-06-yieldy/blob/34774d3f5e9275978621fd20af4fe466d195a88b/src/contracts/BatchRequests.sol#L57 https://github.com/code-423n4/2022-06-yieldy/blob/34774d3f5e9275978621fd20af4fe466d195a88b/src/contracts/BatchRequests.sol#L37

Vulnerability details

Impact

The BatchRequests.removeAddress logic is wrong and it will produce a denial of service.

Proof of Concept

Removing the element from the array is done using the delete statement, but this is not the proper way to remove an entry from an array, it will just set that position to address(0).

Append dummy data:

• addAddress('0x0000000000000000000000000000000000000001')
• addAddress('0x0000000000000000000000000000000000000002')
• addAddress('0x0000000000000000000000000000000000000003')
• getAddresses() => address[]: 0x0000000000000000000000000000000000000001,0x0000000000000000000000000000000000000002,0x0000000000000000000000000000000000000003

Remove address:

• removeAddress(0x0000000000000000000000000000000000000002) (or 0x0000000000000000000000000000000000000003)
• getAddresses() => address[]: 0x0000000000000000000000000000000000000001,0x0000000000000000000000000000000000000000,0x0000000000000000000000000000000000000003

Service is denied because it will try to call canBatchContracts to address(0).

Recommended Mitigation Steps

• To remove an entry in an array you have to use pop and move the last element to the removed entry position.

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/8400e637d9259b7917bde259a5a2fbbeb5946d45/src/contracts/Yieldy.sol#L212

Vulnerability details

Impact

The security of the Yieldy contract is delegated to the compiler used.

Proof of Concept

The allowance of an account does not have to reflect the real balance of an account, however in the transferFrom method, it is the value that is checked in order to verify that the user has enough balance to make the transfer.

    function transferFrom(
        address _from,
        address _to,
        uint256 _value
    ) public override returns (bool) {
        require(_allowances[_from][msg.sender] >= _value, "Allowance too low");

However, the real balance of the Yieldy contract is based on the calculation made by the creditsForTokenBalance method, so an underflow could be made in the subtraction of the balance of the from account.

        uint256 creditAmount = creditsForTokenBalance(_value);
        creditBalances[_from] = creditBalances[_from] - creditAmount;
        creditBalances[_to] = creditBalances[_to] + creditAmount;
        emit Transfer(_from, _to, _value);

This means that the security of the contract is delegated to the checks added by the compiler depending on the pragma used, it must be taken into account that these checks may appear and disappear in future versions of the compiler, so they must be checked at the level of smart contracts.

Affected source code:

• Yieldy.sol#L212

Recommended Mitigation Steps

• Check that the from account has a creditAmount balance.

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/8400e637d9259b7917bde259a5a2fbbeb5946d45/src/contracts/Yieldy.sol#L268

Vulnerability details

Proof of Concept

Using the burn() function of Yieldy, an address with MINTER_BURNER_ROLE can burn an arbitrary amount of tokens from any address.

We believe this is unnecessary and poses a serious centralization risk.

A malicious or compromised MINTER_BURNER_ROLE address can take advantage of this.

Recommended Mitigation Steps

Consider removing the MINTER_BURNER_ROLE and change burn() function to:

    function burn(uint256 _amount) external override 
    {
        _burn(_msgSender(), _amount);
    }

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/8400e637d9259b7917bde259a5a2fbbeb5946d45/src/contracts/Yieldy.sol#L38 https://github.com/code-423n4/2022-06-yieldy/blob/8400e637d9259b7917bde259a5a2fbbeb5946d45/src/contracts/Yieldy.sol#L61-L62

Vulnerability details

Impact

A deprecated method is used in Yieldy contract.

Proof of Concept

In Yieldy contract the method _setupRole is used, and and it is explicitly marked as deprecated by OpenZeppelin.

• NOTE: This function is deprecated in favor of {_grantRole}.

Affected source code:

• Yieldy.sol#L38
• Yieldy.sol#L61-L62

Recommended Mitigation Steps

• The method _setupRole must be changed to _grantRole.

Reduce math operations

Use scientific notation (e.g. 10e17) rather than exponentiation (e.g. 10**18)

Change 10**3 to 10e2:

• LiquidityReserveStorage.sol#L9

Save storage slots

It's possible to save storage slots reorganizing the following contracts:

• Move isReserveEnabled close to stakingContract (or other address type) in LiquidityReserveStorage.sol#L11

• Move decimal close to stakingContract in YieldyStorage.sol#L23

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next (require pragma upgrade).

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Affected source code:

• ERC20Upgradeable.sol#L191
• ERC20Upgradeable.sol#L247
• ERC20Upgradeable.sol#L275-L283
• ERC20Upgradeable.sol#L305
• ERC20Upgradeable.sol#L328-L333
• ERC20Upgradeable.sol#L362-L363

Avoid use Counter to only increase one variable

The Counter contract is only used to increase a variable and involves unnecessary cost overruns.

Affected source code:

• ERC20PermitUpgradeable.sol#L121-L129

unckecked keyword

It's possible to save gas using the unckecked keyword. This will avoid the required checks to ensure that the variable won't overflow.

Reference:

• https://docs.soliditylang.org/en/v0.8.0/control-structures.html#checked-or-unchecked-arithmetic

Affected source code:

• Yieldy.sol#L192
• Yieldy.sol#L212
• Staking.sol#L716

Use right type for avoid casting

• Change the mapping type to IStaking in BatchRequests.sol#L9