Lighthouse NFT Staking - Seascape

Testing revealed that NFTs will always be burned when unstaked due to an error in the StakeNft.sol contract, which burns the token when the variable burn is set to false. Users might not be aware of this when staking their NFT, thus resulting in the loss of their asset.

Code Location

LightHouseStake.sol Lines #151-166

 function unstake(uint256 sessionId, uint256 nftId) external {
        address staker = msg.sender;
        Session storage session = sessions[sessionId];
        require(session.rewardPool > 0, "session does not exist");

        Player storage playerChallenge = playerParams[sessionId][nftId];
        require(playerChallenge.player != msg.sender, "forbidden");

        StakeNft handler = StakeNft(stakeHandler);
        handler.claim(sessionId, staker);

        handler.unstake(sessionId, staker, nftId, false);
        delete playerParams[sessionId][nftId];

        emit Unstake(staker, sessionId, nftId);
    }

StakeNft.sol Lines #79-99

 function unstake(uint key, address stakerAddr, uint id, bool burn)
        external
        nonReentrant
    {
        address stakeToken = periods[msg.sender][key].stakeToken;
        IERC721 nft = IERC721(stakeToken);

        require(nft.ownerOf(id) == address(this), "not owned");
        require(owners[msg.sender][key][id] == stakerAddr);

        delete owners[msg.sender][key][id];
        delete weights[msg.sender][key][id];

        if (burn) {
            nft.safeTransferFrom(address(this), stakerAddr, id);
        } else {
            nft.safeTransferFrom(address(this), 0x000000000000000000000000000000000000dEaD, id);
        }

        withdraw(key, stakerAddr, weights[msg.sender][key][id]);
    }

SOLVED: The Seascape team amended the contracts to remove the burning functionality. Every time a user unstakes their ERC721, it will not be burned, but transferred to them.

\newpage

Within the addSession function of LighthouseStake.sol, the startTime of the session needs to be after the current timestamp, thus in the future. However, this function calls the newPriod function of StakeNft.sol which subsequently calls the newStakePeriod of Stake.sol. Within this last smart contract's function, a check is enforced that the startTime is before or equal to the current timestamp, thus in the past. This is contradictory as the functions are called one after the other, and this will result in the session not being created.

Code Location

LighthouseStake.sol Lines #67-104

[snip]
require(
    rewardPool > 0 && period > 0 && startTime > block.timestamp,
    "zero_value"
);
[snip]

stakeNft.sol Lines #33-56

[snip]
newStakePeriod(key, startTime, endTime, rewardPool);
[snip]

\newpage

Stake.sol Lines #45-51 and #72-91

modifier validStakePeriodParams(uint key, uint startTime, uint endTime, uint rewardPool) {
    require(startTime <= block.timestamp,                   "STAKE_TOKEN: invalid_start");
    require(startTime < endTime,                            "STAKE_TOKEN: invalid_time");
    require(rewardPool > 0,                                 "STAKE_TOKEN: zero_value");
    require(stakePeriods[msg.sender][key].startTime == 0,   "STAKE_TOKEN: period_exists");
    _;
}

function newStakePeriod(
    uint key,               // a unique identifier. could be a session id.
    uint startTime,
    uint endTime,
    uint rewardPool         // if usdc, then decimals is 9 for reward pool.
                            // if cws, then decimals is 18 for reward pool.
)
    internal 
    validStakePeriodParams(key, startTime, endTime, rewardPool)
{
    // Challenge.stake is not null, means that earn is not null too.
    StakePeriod storage period  = stakePeriods[msg.sender][key];
    period.rewardPool           = rewardPool;
    period.startTime            = startTime;
    period.endTime              = endTime;
    period.unit                 = rewardPool * SCALER / (endTime - startTime);
    period.rewardClaimableTime  = startTime;

    emit NewStakePeriod(msg.sender, key, startTime, endTime);
}

SOLVED: The imported contract was amended to ensure that the startTime is in the future.

\newpage

During the audit, it was discovered that all the public functions within the smart contract LighthouseStake.sol such as stake, unstake and claim did not implement protections against reentrancy attacks. To protect against reentrancy attacks, it may be necessary to use a mutex. By using this lock, an attacker can no longer exploit the withdrawal function with a recursive call. OpenZeppelin has its own mutex implementation called ReentrancyGuard which provides a modifier to any function called nonReentrant that protects the function against re-entrancy attacks.

Code Location

The affected functions were in the file LighthouseStake.sol

• stake
• unstake
• claim

SOLVED: The nonReentrant modifier was added to highlighted functions.

\newpage

Within the addSession function of the LighthouseStake contract, one require statement does not produce the appropriate error message. Three different conditions are checked in this statement, and the error message only effectively describes two of them.

Code Location

LighthouseStake.sol - Lines #80-83

require(
    rewardPool > 0 && period > 0 && startTime > block.timestamp,
    "zero_value"
);

SOLVED: The Seascape team amended the code as suggested.

\newpage

It was identified that the tests included within the repository were not sufficient. Should Seascape have implemented more thorough unit tests, certain issues highlighted within this report would have been identified.

ACKNOWLEDGED: The Seascape team did not implement more extensive test cases for NFT staking contracts.

\newpage

Within the LigthouseStake contract, the stake function is described as taking a signature and the amount of assets staked. However, the function definition does only accept the sessionId and nftId parameters. Furthermore, the comment description of the unstake and claim functions do not reflect the parameters passed to the function and their logic.

Should the code called by these functions require these parameters, they would not be provided, and the code might not work as expected. Furthermore, this reduces readability of the code and could introduce errors in code that depend on this smart contract.

LighthouseStake.sol Lines #106-146

/// @notice Stake an  and some token.
/// For the first time whe user deposits his :
///     It receives  id, signature and amount of staking.
function stake(uint256 sessionId, uint256 nftId) external 
[snip]

LighthouseStake.sol Lines #148-166

/// @notice Unstake nft. If the challenge is burning in this sesion
/// then burn nft.
/// @dev data variable is not used, but it is here for following the ZombieFarm architecture.
function unstake(uint256 sessionId, uint256 nftId) external 
[snip]

SOLVED: The Seascape teamamended the comment to reflect the functionality of highlighted functions.

\newpage