Yieldy contest - m_Rassska's results

Lines of code

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

Vulnerability details

Impact

.transfer() overrided in Yieldy.sol does return a boolean value representing the status of the current tx. Since though, it needs to be handled properly.

• Everything is great, but what if the .transfer() ended up by failing?

  • warmpUpInfo about the _recipient will be cleared without transfering credits. And the transfer event also will be emitted causing problems off-chain.

Proof of Concept

• contracts/Staking.sol

  • function claim(address _recipient) public {
        Claim memory info = warmUpInfo[_recipient];
        if (_isClaimAvailable(_recipient)) {
            delete warmUpInfo[_recipient];
    
            if (info.credits > 0) {
                IYieldy(YIELDY_TOKEN).transfer(
                    _recipient,
                    IYieldy(YIELDY_TOKEN).tokenBalanceForCredits(info.credits)
                );
            }
        }
    }

Tools Used

• Oyente && Mythril
• Manual Analysis

Recommended Mitigation Steps

• It's really great that you're using safeTransfer for STAKING_TOKEN which checks it underneath, but here, put at least the require statement.

• Marked as a medium severity because in this audit from Hacken the similar case was considered as a medium severity.

Table of contents

• [G-01] Use custom errors
• [G-02] Consider multiple reverts instead of complex one

<br/>

[G-01] Use custom errors <a name="G-01"></a>

PROBLEM

• It's better to define some custom errors or some custom code-signs.

PROOF OF CONCEPT

• ./Staking.sol

      - "Invalid address"
      - "Must enter valid amount"
      - "Staking is paused"
      - "Must have valid amount"
      - "Insufficient Balance"
      - "Unstaking is paused"
      - "Not enough funds in reserve"
      - "Invalid amount"
      - "Invalid Curve Pool"
  

• ./Yieldy.sol

      - "Already Initialized"
      - "Invalid address"
      - "Can't rebase if not circulating"
      - "Invalid change in supply"
      - "Not enough funds"
      - "Allowance too low"
      - "Mint to the zero address"
      - "Max supply"
      - "Burn from the zero address"
      - "Not enough balance"

TOOLS USED

• Manual Analysis

MITIGATION

• Define custom errors. Or use some sings like: "L1" as a reverting message which will be replaced by corresponding on off-chain.

<br/>

[G-02] Consider multiple reverts instead of complex one<a name="G-02"></a>

PROBLEM

• Since the AND OR opcodes cost some gas, it's better to avoid them by simply invoking require statements one by one.

PROOF OF CONCEPT

• ./Staking.sol

      - require(
          _stakingToken != address(0) &&
              _yieldyToken != address(0) &&
              _tokeToken != address(0) &&
              _tokePool != address(0) &&
              _tokeManager != address(0) &&
              _tokeReward != address(0) &&
              _liquidityReserve != address(0),
          "Invalid address"
      );

TOOLS USED

• Manual Analysis

MITIGATION

• Simply replace complex requires

<br/>