Yieldy contest - MiloTruck's results

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/Staking.sol#L319-L327

Vulnerability details

In Staking.sol, the function _requestWithdrawalFromTokemak() will always fail when _amount is larger than the current balance of the Staking contract.

Impact

The functionality of unstakeAllFromTokemak() is affected as it might call _requestWithdrawalFromTokemak() with _amount higher than the contract's current balance.

Proof of Concept

_requestWithdrawalFromTokemak() is implemented as follows:

src/contracts/Staking.sol:
 319:        function _requestWithdrawalFromTokemak(uint256 _amount) internal {
 320:            ITokePool tokePoolContract = ITokePool(TOKE_POOL);
 321:            uint256 balance = ITokePool(TOKE_POOL).balanceOf(address(this));
 322:    
 323:            // the only way balance < _amount is when using unstakeAllFromTokemak
 324:            uint256 amountToRequest = balance < _amount ? balance : _amount;
 325:    
 326:            if (amountToRequest > 0) tokePoolContract.requestWithdrawal(_amount);
 327:        }

At line 324, amountToRequest is set to either balance or _amount, depending on which is smaller. However, at line 326, it calls requestWithdrawal() with _amount as the parameter instead of amountToRequest. As such, should balance be larger than _amount, the function will try to withdraw more tokens than it currently owns, causing it to revert.

Recommended Mitigation Steps

Change line 326 to use amountToRequest as the parameter instead:

if (amountToRequest > 0) tokePoolContract.requestWithdrawal(amountToRequest);

Non-Critical Issues

Missing zero-address checks in Yieldy.sol

In Yieldy.sol, all functions except transferFrom() have zero address checks for address parameters. However, transferFrom() is missing zero address checks for _to and _from, making it inconsistent:

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

        uint256 newValue = _allowances[_from][msg.sender] - _value; 
        _allowances[_from][msg.sender] = newValue;
        emit Approval(_from, msg.sender, newValue);

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

        return true;
    }

Impact

Tokens would become inaccessible if _to is set to address(0).

Recommended Mitigation

Consider adding zero-address checks to _to and _from. Other than preventing users from sending tokens to address(0), it would also help to save gas should _from be set to address(0).

event is missing indexed fields

Each event should use three indexed fields if there are three or more fields:

src/contracts/Yieldy.sol:
  15:        event LogSupply(
  16:            uint256 indexed epoch,
  17:            uint256 timestamp,
  18:            uint256 totalSupply
  19:        );

  21:        event LogRebase(uint256 indexed epoch, uint256 rebase, uint256 index);

src/contracts/Staking.sol:
  36:        event LogSetCurvePool(address indexed curvePool, int128 to, int128 from);

Table of Contents

• Arithmetics: ++i costs less gas compared to i++ or i += 1
• Arithmetics: Use != 0 instead of > 0 for unsigned integers
• Visibility: public functions can be set to external
• Errors: Use custom errors instead of revert strings
• Errors: Use multiple require statements instead of &&
• Errors: Duplicate require()/revert() checks should be refactored to a modifier
• Errors: Remove redundant require statements in Yieldy.sol
• Unnecessary definition of variables
• Use variables cached on the stack instead of reading from storage
• Cache and read storage variables from the stack
• Inline function contractBalance() in Staking.sol
• Parameter validation should always be done first

Arithmetics: ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2, thus it costs more gas.

The same logic applies for --i and i--.

Consider using ++i instead of i++ or i += 1 in the following instances:

src/contracts/Staking.sol:
 708:        epoch.number++;

Arithmetics: Use != 0 instead of > 0 for unsigned integers

uint will never go below 0. Thus, > 0 is gas inefficient in comparisons as checking if != 0 is sufficient and costs less gas.

Consider changing > 0 to != 0 in these lines:

src/contracts/LiquidityReserve.sol:
 223:        if (amount > 0) IStaking(stakingContract).unstake(amount, false);

src/contracts/Yieldy.sol:
  83:        require(_totalSupply > 0, "Can't rebase if not circulating");
  96:        require(rebasingCreditsPerToken > 0, "Invalid change in supply");

src/contracts/Staking.sol:
 118:        require(_recipient.amount > 0, "Must enter valid amount");
 305:        requestedWithdrawals.amount > 0 &&
 326:        if (amountToRequest > 0) tokePoolContract.requestWithdrawal(_amount);
 363:        requestWithdrawalAmount > 0;
 392:        if (requestWithdrawalAmount > 0) {
 410:        require(_amount > 0, "Must have valid amount");
 415:        if (yieldyTotalSupply > 0) {
 470:        if (info.credits > 0) {
 533:        if (warmUpBalance > 0) {
 572:        require(_amount > 0, "Invalid amount");
 604:        require(_amount > 0, "Invalid amount");

Visibility: public functions can be set to external

Calls to external functions are cheaper than public functions. Thus, if a function is not used internally in any contract, it should be set to external to save gas and improve code readability.

Consider changing following functions from public to external:

src/contracts/Staking.sol:
 370:        function unstakeAllFromTokemak() public onlyOwner {

Errors: Use custom errors instead of revert strings

Since Solidity v0.8.4, custom errors should be used instead of revert strings due to:

• Cheaper deployment cost
• Lower runtime cost upon revert

Taken from 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 can be defined using of the error statement, both inside or outside of contracts.

Instances where custom errors can be used instead:

src/contracts/LiquidityReserve.sol:
  25:        require(msg.sender == stakingContract, "Not staking contract");
  61:        require(!isReserveEnabled, "Already enabled");
  62:        require(_stakingContract != address(0), "Invalid address");
  94:        require(_fee <= BASIS_POINTS, "Out of range");
 105:        require(isReserveEnabled, "Not enabled yet");
 163:        require(_amount <= balanceOf(msg.sender), "Not enough lr tokens");
 192:        require(isReserveEnabled, "Not enabled yet");
 215:        require(isReserveEnabled, "Not enabled yet");

src/contracts/Yieldy.sol:
  58:        require(stakingContract == address(0), "Already Initialized");
  59:        require(_stakingContract != address(0), "Invalid address");
  83:        require(_totalSupply > 0, "Can't rebase if not circulating");
  96:        require(rebasingCreditsPerToken > 0, "Invalid change in supply");
 187:        require(_to != address(0), "Invalid address");
 190:        require(creditAmount <= creditBalances[msg.sender], "Not enough funds");
 210:        require(_allowances[_from][msg.sender] >= _value, "Allowance too low");
 249:        require(_address != address(0), "Mint to the zero address");
 257:        require(_totalSupply < MAX_SUPPLY, "Max supply");
 279:        require(_address != address(0), "Burn from the zero address");
 286:        require(currentCredits >= creditAmount, "Not enough balance"); 

src/contracts/Staking.sol:
 118:        require(_recipient.amount > 0, "Must enter valid amount");
 143:        require(_claimAddress != address(0), "Invalid address");
 408:        require(!isStakingPaused, "Staking is paused");
 410:        require(_amount > 0, "Must have valid amount");
 572:        require(_amount > 0, "Invalid amount");
 586:        require(reserveBalance >= _amount, "Not enough funds in reserve");
 604:        require(_amount > 0, "Invalid amount");
 644:        require(from == 1 || to == 1, "Invalid Curve Pool");
 676:        require(!isUnstakingPaused, "Unstaking is paused");

Errors: Use multiple require statements instead of &&

Instead of using a single require statement with the && operator, using multiple require statements would help to save runtime gas cost. However, note that this results in a higher deployment gas cost, which is a fair trade-off.

A require statement can be split as such:

// Original code:
require(a && b, 'error');

// Changed to:
require(a, 'error: a');
require(b, 'error: b');

Instances where multiple require statements should be used:

src/contracts/Staking.sol:
  54:        require(
  55:            _stakingToken != address(0) &&
  56:                _yieldyToken != address(0) &&
  57:                _tokeToken != address(0) &&
  58:                _tokePool != address(0) &&
  59:                _tokeManager != address(0) &&
  60:                _tokeReward != address(0) &&
  61:                _liquidityReserve != address(0),
  62:            "Invalid address"
  63:        );

src/contracts/LiquidityReserve.sol:
  44:        require(
  45:            _stakingToken != address(0) && _rewardToken != address(0),
  46:            "Invalid address"
  47:        );

Errors: Duplicate require()/revert() checks should be refactored to a modifier

The following require() statement is repeated in multiple functions in LiquidityReserve.sol:

src/contracts/LiquidityReserve.sol:
 105:        require(isReserveEnabled, "Not enabled yet");
 192:        require(isReserveEnabled, "Not enabled yet");
 215:        require(isReserveEnabled, "Not enabled yet");

Instead of repeatedly using a require statement to check isReserveEnabled, consider using a modifier as such:

modifier reserveEnabled() {
  require(isReserveEnabled, "Not enabled yet");
  _;
}

This would help improve code clarity and reduce deployment gas cost.

Errors: Remove redundant require statements in Yieldy.sol

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks. As such, it is redundant to check if a variable is larger than another before doing a subtraction operation.

For example, in transfer():

src/contracts/Yieldy.sol:
 190:        require(creditAmount <= creditBalances[msg.sender], "Not enough funds");
 191:
 192:        creditBalances[msg.sender] = creditBalances[msg.sender] - creditAmount;

At line 192, the subtraction would automatically revert if creditBalances[msg.sender] is less than creditAmount to prevent an underflow. As such, the require statement at line 190 is redundant and can be removed to reduce deployment and runtime gas costs.

This also occurs in transferFrom() and _burn():

src/contracts/Yieldy.sol:
 210:        require(_allowances[_from][msg.sender] >= _value, "Allowance too low");
 211:
 212:        uint256 newValue = _allowances[_from][msg.sender] - _value; 

 285:        uint256 currentCredits = creditBalances[_address];
 286:        require(currentCredits >= creditAmount, "Not enough balance");
 287:
 288:        creditBalances[_address] = creditBalances[_address] - creditAmount;

Note that an alternative is to keep the require statements and wrap the arithmetic operations in an unchecked block instead, since it would be impossible for an underflow to occur.

Unnecessary definition of variables

Some variables are defined even though they are only used once in their respective functions. To reduce gas cost and contract size, consider ot defining these variables and directly using the expressions on the right.

Instances include:

src/contracts/Staking.sol:
 334:        ITokePool tokePoolContract = ITokePool(TOKE_POOL);
 343:        ITokePool tokePoolContract = ITokePool(TOKE_POOL);
 358:        uint256 nextCycleStart = currentCycleStart + duration;
 391:        ITokeManager tokeManager = ITokeManager(TOKE_MANAGER);
 728:        uint256 tokeBalance = _getTokemakBalance();
 757:        uint256 amountToDeposit = stakingTokenBalance - withdrawalAmount;

src/contracts/LiquidityReserve.sol:
  64:        uint256 stakingTokenBalance = IERC20Upgradeable(stakingToken).balanceOf(
  65:            msg.sender
  66:        );

Use variables cached on the stack instead of reading from storage

In _requestWithdrawalFromTokemak() of Staking.sol, instead of reading TOKE_POOL from storage and casting it to ITokePool again, simply use tokePoolContract instead. This would prevent additional SLOAD calls and save ~100 gas.

The following code:

src/contracts/Staking.sol:
 320:        ITokePool tokePoolContract = ITokePool(TOKE_POOL);
 321:        uint256 balance = ITokePool(TOKE_POOL).balanceOf(address(this));

can be changed to:

src/contracts/Staking.sol:
 320:        ITokePool tokePoolContract = ITokePool(TOKE_POOL);
 321:        uint256 balance = tokePoolContract.balanceOf(address(this));

Similarly, in rebase() of Yieldy.sol, _totalySupply is read from storage twice:

src/contracts/Yieldy.sol:
  82:        uint256 currentTotalSupply = _totalSupply;
  83:        require(_totalSupply > 0, "Can't rebase if not circulating"); 

It can be changed to:

        uint256 currentTotalSupply = _totalSupply;
        require(currentTotalSupply > 0, "Can't rebase if not circulating"); 

In _burn() of Yieldy.sol, creditBalances[_address] is read from storage twice:

src/contracts/Yieldy.sol:
 285:        uint256 currentCredits = creditBalances[_address];
 286:        require(currentCredits >= creditAmount, "Not enough balance"); 
 287:
 288:        creditBalances[_address] = creditBalances[_address] - creditAmount;

It can be changed to:

        uint256 currentCredits = creditBalances[_address];
        require(currentCredits >= creditAmount, "Not enough balance"); 

        creditBalances[_address] = currentCredits - creditAmount;

Cache and read storage variables from the stack

For storage variables that are accessed multiple times in a function, they should be cached first and then read from the stack subsequently. This would save ~100 gas for each additional read (SLOAD after Berlin).

In src/contracts/Yieldy.sol, the variable creditBalances[msg.sender] in transfer():

function transfer(address _to, uint256 _value)
    public
    override
    returns (bool)
{
    require(_to != address(0), "Invalid address");

    uint256 creditAmount = _value * rebasingCreditsPerToken;
    require(creditAmount <= creditBalances[msg.sender], "Not enough funds"); 
    
    creditBalances[msg.sender] = creditBalances[msg.sender] - creditAmount;
    // ...
}

can be changed to:

function transfer(address _to, uint256 _value)
    public
    override
    returns (bool)
{
    require(_to != address(0), "Invalid address");

    uint256 creditAmount = _value * rebasingCreditsPerToken;
    uint256 creditBalance = creditBalances[msg.sender];
    require(creditAmount <= creditBalance, "Not enough funds"); 
    
    creditBalances[msg.sender] = creditBalance - creditAmount;
    // ...
}

In src/contracts/Yieldy.sol, the variable _allowances[_from][msg.sender] in transferFrom():

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

    uint256 newValue = _allowances[_from][msg.sender] - _value; 
    // ...
}

can be changed to:

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

    uint256 newValue = allowance - _value; 
    // ...
}

Inline function contractBalance() in Staking.sol

The internal function contractBalance() is only called once in rebase(), as shown below:

src/contracts/Staking.sol:
 701:    function rebase() public {
 702:        // we know about the issues surrounding block.timestamp, using it here will not cause any problems
 703:        if (epoch.endTime <= block.timestamp) {
 704:            // ...
 709:
 710:            uint256 balance = contractBalance();
 711:            uint256 staked = IYieldy(YIELDY_TOKEN).totalSupply();
 712:
 713:            // ...
 718:        }
 719:    }

Since contractBalance() is an internal function and its logic is relatively simple, it should be inlined. This would help to reduce runtime and deployment gas costs, and improve code readeability.

Consider removing the function contractBalance(), and rebase() can be rewritten as such:

src/contracts/Staking.sol:
 701:    function rebase() public {
 702:        // we know about the issues surrounding block.timestamp, using it here will not cause any problems
 703:        if (epoch.endTime <= block.timestamp) {
 704:            // irrelevant code removed...
 709:
 710:            uint256 balance = IERC20Upgradeable(STAKING_TOKEN).balanceOf(address(this)) + _getTokemakBalance();
 711:            uint256 staked = IYieldy(YIELDY_TOKEN).totalSupply();
 712:
 713:            // irrelevant code removed...
 718:        }
 719:    }

Parameter validation should always be done first

In the function enableLiquidityReserve() of LiquidityReserve.sol, the zero address check for _stakingContract is placed after the check for isReserveEnabled:

src/contracts/LiquidityReserve.sol
  57:        function enableLiquidityReserve(address _stakingContract)
  58:            external
  59:            onlyOwner
  60:        {
  61:            require(!isReserveEnabled, "Already enabled");
  62:            require(_stakingContract != address(0), "Invalid address");

However, the _stakingContract != address(0) check should be done first. Should the check fail, it would avoid reading from storage, thus reducing runtime gas cost.