Goldivault Diff - Goldilocks

All the files in scope currently use floating pragma version ^0.8.20, which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.0, and less than 0.9.0.

It is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

It is recommended to lock the pragma version to the same version used during development and testing.

SOLVED: The Goldilocks team solved the issue as recommended.

The Goldivault4626 contract provides functionality for staking and unstaking Yield Tokens. When a user redeems their principal, part of the process involves unstaking YT so that YT can be burned or transferred.

When a user calls a function that requires unstaking (e.g. redeemOwnership), the contract determines how many tokens to unstake by calling unstakableYT function. The returned value from unstakableYT is passed to _unstakeYT, which ultimately adjusts the ytStaked[user] balance and transfers the tokens out:

 function _unstakableYT(address user, uint256 unstakeAmount) internal view returns (uint256) {
    uint256 _ytStaked = ytStaked[user];
    if(_ytStaked == 0) {
      return 0;
    }
    else if(unstakeAmount > _ytStaked) {
      return unstakeAmount - _ytStaked;
    }
    else {
      return unstakeAmount;      
    }
  }

When unstakeAmount exceeds ytStaked, the function returns unstakeAmount - ytStaked. This can be misleading because returning the difference does not represent what can be unstaked, rather, it’s the shortfall between the requested amount and the user’s staked balance. 

Subsequent calls to _unstakeYT will revert anyway if that returned value exceeds the user’s actual stake:

  function _unstakeYT(uint256 amount) internal {
    if(amount > ytStaked[msg.sender]) revert InvalidUnstake();
    ytStaked[msg.sender] -= amount;
    totalYtStaked -= amount;
    SafeTransferLib.safeTransfer(yt, msg.sender, amount);
    emit YTUnstake(msg.sender, amount);
  }

It is recommended to remove the mentioned function.

SOLVED: The Goldilocks team solved the issue. The _unstakableYT function was removed.

The deposit function implemented in the Goldivault4626 contract, replicates the same logic that the internal _deposit function, which can introduce unnecessary redundancy and potential for inconsistencies. In both places, the code does the following:

    if(amount == 0) revert InvalidDeposit();
    SafeTransferLib.safeTransferFrom(depositToken, msg.sender, address(this), amount);
    ERC20(depositToken).approve(depositVault, amount);
    ERC4626(depositVault).deposit(amount, address(this));
    depositTokenAmount += amount;
    OwnershipToken(ot).mintOT(msg.sender, amount);
    YieldToken(yt).mintYT(msg.sender, amount);
    _updateClaimableUnderlying(msg.sender);
    _stakeYT(amount);
    emit Deposit(msg.sender, amount);

When other parts of the contract (for example, buyYT) require depositing, they already call the internal _deposit directly, re-running the same series of steps.

It is recommended to streamline the deposit process by making the public deposit function internally invoke _deposit, removing duplicate logic.

SOLVED: The Goldilocks team solved the issue as recommended. The internal _deposit function was separated from the external one.

While most state-changing functions in the contract include a nonReentrant modifier, the stakeYT and unstakeYT functions do not, which can open a window for potential reentrancy attacks.

  /// @notice Stakes YT
  function stakeYT(uint256 amount) external {
    _updateClaimableUnderlying(msg.sender);
    _stakeYT(amount);
  }

  /// @notice Unstakes YT
  function unstakeYT(uint256 amount) external {
    _updateClaimableUnderlying(msg.sender);
    uint256 unstakableAmount = _unstakableYT(msg.sender, amount);
    _unstakeYT(unstakableAmount);
  }

It is recommended to add the nonReentrant modifier to stakeYT and unstakeYT functions.

SOLVED: The Goldilocks team solved the issue as recommended.

The project is using Solidity version greater than 0.8.18, which supports named mappings. Using named mappings can improve the readability and maintainability of the code by making the purpose of each mapping clearer. This practice will enhance code readability and make the purpose of each mapping more explicit.

Consider refactoring the mappings to use named arguments.

For example, on Goldivault4626, instead of declaring:

mapping(address => uint256) public ytStaked; 

The mapping could be declared as: 

mapping(address user => uint256 amount) public ytStaked; 

SOLVED: The Goldilocks team solved the issue as recommended.