GLIF LpPlus & RWTFuture Contracts Security Review Report

The Merkle root and the staking snapshot for a specific window are set using the flow LpPlusMerkleHelper.setWindowRoot() -> LpPlus.commitWindow(). The function LpPlusMerkleHelper.setWindowRoot() is responsible for computing the necessary data, while LpPlus.commitWindow() only stores that data on-chain.

Inside LpPlusMerkleHelper.setWindowRoot() the value stakingTotalYbt is computed by the internal function _computeTwaStakingTotalYbt(). This stakingTotalYbt is then stored in the window as stakingSnapshot.stakingYbtTwab and used as the denominator when allocating FIL to users. In other words, for a given window, if a user has a time-weighted average YBT balance ybt, their FIL allocation is:

 stakingSnapshot.totalFilToAllocate
 
 ------------------------------------------ * ybt
 
 stakingSnapshot.stakingYbtTwab

FIL tokens.

There is a flaw in _computeTwaStakingTotalYbt() at line 317: totalYbt is initialized with _lastVars.lastYbt, which represents the closingTotalYbt of the previous staking snapshot. This is inconsistent with _computeTwaValues(), which computes each user's time-weighted average YBT balance and does not include the previous snapshot's closingTotalYbt. As a result, stakingYbtTwab can be overstated and an individual user's allocated FIL will be lower than expected.

The allocated FIL should be correctly distributed among the total balance.

Inside LpPlusMerkleHelper._computeTwaStakingTotalYbt(), the function _binarySearchWindowId() is invoked with _windowStart passed as the _windowId argument:

uint256 low = _binarySearchWindowId(
    _windowStart,   /// <== @audit this is a timestamp, not a windowId
    actions,
    _compareExclusive
);

This is incorrect because _windowStart represents the timestamp of the last staking snapshot, whereas _binarySearchWindowId() expects a window ID, which should be a much smaller index value - not a timestamp.

Due to this mismatch, _binarySearchWindowId() returns an invalid result, causing _computeTwaStakingTotalYbt() to compute stakingTotalYbt incorrectly. As a consequence, setWindowRoot() will produce an incorrect staking total for the window, directly impacting the FIL allocation for users.

The function should use the _windowId instead of _windowStart.

In the _mintRWTFutures() function, there is a check to ensure that the FIL reserves are sufficient for the allocations in this mint.

// Check if FIL reserves are sufficient for this allocation
require(
    totalAllocatedFil <= address(this).balance, FilReservesDepleted(totalAllocatedFil, address(this).balance)
);

However, totalAllocatedFil in the above code is only a memory variable used within the _mintRWTFutures() function, so it cannot ensure that the contract's FIL reserves are sufficient for all RWTFuture allocations that will be executed.

Therefore, after using claim() and receiving RWTFuture tokens, users may be unable to execute them before expiration due to insufficient FIL reserves, resulting in a loss of claims.

Additionally, claims made through claimAndExecute(), which does not trigger _mintRWTFutures(), are missing a FIL reserve. This can also cause unexecuted claims from the claim() function to be unable to execute due to insufficient FIL tokens.

The total allocation of unexecuted claims should be checked against the available FIL when creating new claims (for example, in the claimBatch() function) to ensure they are executable in time.

The LpPlus NFT contains a balance of YBT and RWT and the contract has functions to respectively add and remove these.

The contract also implements a minimum balance for both YBT and RWT (initially set to 1e18), so if adding or removing would lead to a non-zero balance of less than the minimum, it would revert:

uint256 newBalance = vaultBalance - _RWTAmount;
uint256 minimumRWTBalance_ = $.minimumRWTBalance;
require(
    newBalance >= minimumRWTBalance_ || newBalance == 0,
    InvalidBalance(_tokenId, newBalance, minimumRWTBalance_)
);

Only if the remove function is called with the exact amount that is the full balance, the resulting newBalance would be 0 and it would be allowed.

However, due to this check, it becomes possible for an attacker to DoS full removal of YBT or RWT by front-running the transaction and donating 1 wei of the corresponding token using the permissionless add function.

For example:

• A user has an NFT with token ID 1 and 100 YBT and 100 RWT.
• The user calls removeRWT(1, 100e18)
• The attacker front-runs this call and calls addRWT(1, 1), adding 1 wei to the NFT's YBT balance.
• The user's function call will now revert, because the calculated newBalance will be equal to 1 wei.

Consider adding the leftover balance to the amount to send when removing YBT or RWT.

The LpPlus contract has a constant DEFAULT_FUTURE_EXPIRATION_PERIOD which is set to 180 days. However, the natspec comment states that it is 6 months, which is not the same and would be more accurately equal to 26 weeks or 182 days.

/// @dev The default future expiration period in seconds (6 months). RWT Futures lifespan.
uint256 private constant DEFAULT_FUTURE_EXPIRATION_PERIOD = 6 * 30 days;

We recommend to consider correcting either the comment or the constant's value.

In the function mint of the LpPlus contract, the function parameter _to is normalised into the stack variable to on line 173.

However, the raw _to address is still used as input for the modifier validAddress(_to) on line 168.

The modifier does not perform normalisation.

function mint(address _to, uint256 _referrerTokenId, uint256 _ybtToStake, uint256 _rwtToStake)
    external
    virtual
    whenNotPaused
    validAddress(_to)
    returns (uint256 tokenId)
{
    [..]
}
 
modifier validAddress(address _address) {
    require(_address != address(0), ZeroAddress());
    _;
}

We recommend normalising the address in the modifier:

modifier validAddress(address _address) {
--  require(_address != address(0), ZeroAddress());
++  require(_address.normalize() != address(0), ZeroAddress());
    _;
}

In the contract LpPlus, there are numerous functions that allow the owner to change the configuration state variables. However, none of the functions emit events with the new values.

This makes off-chain tracking of the state cumbersome, as it is not always evident from the root transaction that a change was made (such as with Safe multi-sig wallets).

We recommend to add specific events for important state configuration changes that communicate the new value to off-chain indexers.

In the function withdrawFilFunds, the owner can withdraw FIL from the contract. The function does some validation in the form of a balance check against the specified amount.

However, this check is redundant, as the same check is already performed in the sendValue function that is subsequently used to send the FIL:

function sendValue(address payable _recipient, uint _amount) internal {
    if (address(this).balance < _amount) revert InsufficientFunds();
 
    (bool success, ) = _recipient.call{value: _amount}("");
    if (!success) revert CallFailed();
}

Source: fevmate/contracts/utils/FilAddress.sol at 6a80e989847fd563df21360176cbfd5d08aaabbb · wadealexc/fevmate

We recommend to remove the redundant check in favour of gas savings and redundancy.

In the function mint of LpPlus, the _to parameter is normalised into the local to variable. The latter is then used in subsequent actions, function calls and events.

However, only on line 180 there is an event that still emits the unnormalised version using _to.

function mint(address _to, uint256 _referrerTokenId, uint256 _ybtToStake, uint256 _rwtToStake)
    external
    virtual
    whenNotPaused
    validAddress(_to)
    returns (uint256 tokenId)
{
    [..]
 
    emit CardMinted(tokenId, msg.sender, _to, _referrerTokenId);
 
    [..]
}

We recommend using to in the event instead.

In the RWTFuture contact, there is an owner function setLpPlus to change the configuration state of the contract.

However, this function does not emit an event with the new value.

This makes off-chain tracking of the state cumbersome, as it is not always evident from the root transaction that a change was made (such as with Safe multi-sig wallets).

function setLpPlus(ILpPlus _lpPlus) external virtual onlyOwner {
    require(address(_lpPlus) != address(0), ZeroAddress());
    RWTFutureStorage storage $ = _getStorage();
    $.lpPlus = ILpPlus(address(_lpPlus).normalize());
}

We recommend to add a corresponding event to the function.

In the function _executeBatch, the RWTFuture holder can execute their the strike price and purchase FIL from the contract. The function does some validation in the form of a balance check against the specified amount.

However, this check is redundant, as the same check is already performed in the sendValue function that is subsequently used to send the FIL:

function _executeBatch(
    uint256[] memory _rwtFutureTokenIds,
    uint256[] memory _strikePrices,
    uint256[] memory _allocatedFils,
    address _receiver,
    LpPlusStorage storage $
) internal {
    [..]
 
    require(address(this).balance >= allocatedFil, InsufficientFILBalance(allocatedFil, address(this).balance));
 
    $.RWTToken.transferFrom(msg.sender, $.treasury, requiredRwt);
 
    payable(_receiver).sendValue(allocatedFil);
}

Source: fevmate/contracts/utils/FilAddress.sol at 6a80e989847fd563df21360176cbfd5d08aaabbb · wadealexc/fevmate

We recommend to remove the redundant check in favour of gas savings and redundancy.

In the function _claimBatch, a holder of the LpPlus NFT can claim an RWTFuture using Merkle proofs. The function takes 3 different arrays and checks the length of each:

require(length > 0, ZeroAmount());
require(_userSnapshots.length > 0, ZeroAmount());
require(_proofs.length > 0, ZeroAmount());
 
require(_userSnapshots.length == length, InvalidLengths(_userSnapshots.length, length));
require(_proofs.length == length, InvalidLengths(_proofs.length, length));

However, the non-zero checks on the _userSnapshots and _proofs is redundant, as they are required to be equal to length and length is greater than 0.

We recommend to remove the redundant check in favour of gas savings and redundancy.

In the protocol, the LpPlus contract is the only contract that is allowed to burn RWTFuture NFTs and it is only done in the execute function.

One requirement in the function, is that the RWTFuture has not expired for it to be executed:

require(
    block.timestamp <= position.expirationDate, RWTFutureExpired(rwtFutureTokenId, position.expirationDate)
);

Even though this is correct, it does make burning expired (and thus useless) RWTFuture NFTs impossible.

Consider allowing the burning of expired RWTFuture NFTs, which would not need a subsequent call to _executeBatch.