Require statement reverting transaction everytime

Question

something strange is going on. I'm testing some contracts on remix EVM. I have some pretty basic NFT staking contracts that works fine when comes to staking and transfering the token. However, if I try to execute the unstake function, the transaction gets reverted saying that the the require conditions are not passing. However, and more strange is that if I call the functions inside the require separately the condition is true!

I don't know whats going on at this point, so please any advice or help would be much appreciated. Currently I have three contracts (ERC20, ERC721, ERC721staking) with all functions working just right except for the unstake function.

These are my contracts: Energy.sol (ERC20):

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/access/Ownable.sol";

/* Simple ERC20 token contract to issue rewards */
contract Energy is ERC20, Ownable {
    mapping(address => bool) minters;

    constructor() ERC20("ENERGY", "NRG") {
        _mint(msg.sender, 100 * 10**decimals());
    }

    modifier isMinter() {
        require(minters[msg.sender], "Caller is not authorized to mint!");
        _;
    }

    function mintRewards(address to, uint256 amount) external isMinter {
        _mint(to, amount * 10**decimals());
    }

    function addMinter(address account) public onlyOwner {
        minters[account] = true;
    }
}

Fuel.sol (ERC721)

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts/token/ERC721/extensions/ERC721Burnable.sol";
import "@openzeppelin/contracts/access/Ownable.sol";
import "@openzeppelin/contracts/utils/Counters.sol";

contract Fuel is ERC721, ERC721Burnable, Ownable {
    using Counters for Counters.Counter;

    Counters.Counter private _tokenIdCounter;

    constructor() ERC721("Fuel", "FUEL") {}

    function safeMint(address to) public onlyOwner {
        uint256 tokenId = _tokenIdCounter.current();
        _tokenIdCounter.increment();
        _safeMint(to, tokenId);
    }
}

Generator.sol (staking):

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
import "@openzeppelin/contracts/access/Ownable.sol";
import "./Energy.sol";
import "./Fuel.sol";

struct Loader {
    uint256[] fuelIds;
    mapping(uint256 => uint256) loadBlock;
}

contract Generator is Ownable, ReentrancyGuard, IERC721Receiver {
    Fuel fuel;
    Energy energy;

    uint256 rewardsPerBlock = 5;

    mapping(address => Loader) loaders;

    // Enumeration of fuelIds staked indexes of a loader
    mapping(address => mapping(uint256 => uint256)) public fuelIdIndex;

    // tracks owner of a fuelId
    mapping(uint256 => address) public loaderOf;

    constructor(address _fuel, address _energy) {
        fuel = Fuel(_fuel);
        energy = Energy(_energy);
    }

    function stake(uint256 fuelId) public nonReentrant {
        // safe checks
        require(
            fuel.ownerOf(fuelId) == msg.sender,
            "You're not the owner of this NFT"
        );

        // push new token to staking collection
        loaders[msg.sender].fuelIds.push(fuelId);

        // updates index reference of fuelId
        uint256 totalFuel = loaders[msg.sender].fuelIds.length;
        fuelIdIndex[msg.sender][fuelId] = totalFuel - 1;

        // inits staking block
        loaders[msg.sender].loadBlock[fuelId] = block.number;

        // add it to reference
        loaderOf[fuelId] = msg.sender;

        fuel.safeTransferFrom(address(msg.sender), address(this), fuelId);
    }

    function unstake(uint256 fuelId) public nonReentrant {
        // safe checks
        require(ownedByThis(fuelId), "This fuel is not being loaded here!");

        require(
            _loaderOf(fuelId) == address(msg.sender),
            "You haven't loaded this fuel here!"
        );

        uint256 lastFuelIndex = loaders[msg.sender].fuelIds.length - 1;
        uint256 fuelIndex = fuelIdIndex[msg.sender][fuelId];

        // swap current fuelId to last position
        if (lastFuelIndex != fuelIndex) {
            uint256 lastFuelId = loaders[msg.sender].fuelIds[lastFuelIndex];

            loaders[msg.sender].fuelIds[fuelIndex] = lastFuelIndex; // Move the last token to the slot of the to-delete token
            fuelIdIndex[msg.sender][lastFuelId] = fuelIndex; // Update the moved token's index
        }

        // remove the last element from mapping and array
        delete fuelIdIndex[msg.sender][fuelId];
        delete loaders[msg.sender].fuelIds[lastFuelIndex];

        delete loaders[msg.sender].loadBlock[fuelId];
        delete loaderOf[fuelId];

        // Transfer back to the owner
        fuel.safeTransferFrom(address(this), address(msg.sender), fuelId);
        claim(fuelId);
    }

    function claim(uint256 fuelId) public {
        // safe checks
        require(ownedByThis(fuelId), "This fuel is not being loaded here!");

        require(
            _loaderOf(fuelId) == address(msg.sender),
            "You haven't loaded this fuel here!"
        );

        uint256 rewardsToClaim = getPendingRewards(msg.sender, fuelId);
        energy.mintRewards(msg.sender, rewardsToClaim);

        loaders[msg.sender].loadBlock[fuelId] = block.number;
    }

    function claimAll() public nonReentrant {
        // safe checks
        require(
            loaders[msg.sender].fuelIds.length > 0,
            "You have no fuel loaded here!"
        );

        uint256 totalFuelLoaded = totalFuelLoadedBy(msg.sender);

        for (uint256 i = 0; i < totalFuelLoaded; i++) {
            uint256 fuelId = loaders[msg.sender].fuelIds[i];
            claim(fuelId);
        }
    }

    function getPendingRewards(address account, uint256 fuelId)
        public
        view
        returns (uint256)
    {
        uint256 loadBlock = loaders[account].loadBlock[fuelId];
        uint256 blocksElapsed = block.number - loadBlock;

        return blocksElapsed * rewardsPerBlock;
    }

    function getAllPendingRewards() public view returns (uint256) {
        uint256 totalFuelLoaded = totalFuelLoadedBy(msg.sender);

        uint256 totalRewards = 0;
        for (uint256 i = 0; i < totalFuelLoaded; i++) {
            uint256 fuelId = loaders[msg.sender].fuelIds[i];
            totalRewards += getPendingRewards(msg.sender, fuelId);
        }

        return totalRewards;
    }

    function _loaderOf(uint256 fuelId) public view returns (address) {
        return loaderOf[fuelId];
    }

    function totalFuelLoadedBy(address account) public view returns (uint256) {
        return loaders[account].fuelIds.length;
    }

    function generatorAddress() public view returns (address) {
        return address(this);
    }

    function ownedByThis(uint256 fuelId) public view returns (bool) {
        return address(fuel.ownerOf(fuelId)) == address(this);
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 fuelId,
        bytes calldata data
    ) external override returns (bytes4) {
        return this.onERC721Received.selector;
    }
}

If you want to test the flow (and I hope you do) be sure to deploy first the Fuel and Energy contracts, then use the address of the contracts as constructor arguments when deploying the Generator contract. Then approveForAll the generator address in the fuel instance, mint some nfts, stake in the generator contract and try to unstake. Every function will work just fine but the unstake function.

Thanks again for any help!

As opposed to calling `externally`, how did you call the function? — Tahlil, Aug 04 '22 at 18:05
Can you elaborate on the two way you called the `unstake` function? — Tahlil, Aug 05 '22 at 04:59
Sorry, I meant to say "However, and more strange is that if I call the functions inside the require separately the condition is true!", now the question is updated. — yieniggu, Aug 11 '22 at 22:03
Is the fault happen when you test it in remix? Also i recommand that for us and for you, test only the function that are executing with the faulty function, its hard for us to go through your entire code and also for you its always better to first test code in the smallest unit possible. — NGDeveloper, Aug 11 '22 at 22:11

score 0 · Answer 1 · answered Aug 10 '22 at 13:18

Function ownedByThis takes address ownerOf(fuelId) from you Fuel contract, but after you staked your NFT in Generator.sol, now Generator.sol is owner of this NFT, and your require statement with function ownedByThis is not working. Also i added (delete loaderOf[fuelId];) to the very bottom of your Claim function. Before unstake your nft dont forget to use AddMinter funtion for address of Generator.sol contract. Hope i was useful.

Updated code below

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
import "@openzeppelin/contracts/access/Ownable.sol";
import "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
import "./Energy.sol";
import "./Fuel.sol";


contract Generator is Ownable, ReentrancyGuard, ERC721Holder {
Fuel fuel;
Energy energy;

struct Loader {
uint256[] fuelIds;
mapping(uint256 => uint256) loadBlock;
}

uint256 rewardsPerBlock = 5;

mapping(address => Loader) loaders;

// Enumeration of fuelIds staked indexes of a loader
mapping(address => mapping(uint256 => uint256)) public fuelIdIndex;

// tracks owner of a fuelId
mapping(uint256 => address) public loaderOf;

constructor(address _fuel, address _energy) {
    fuel = Fuel(_fuel);
    energy = Energy(_energy);
}

function stake(uint256 fuelId) public nonReentrant {
    // safe checks
    require(
        fuel.ownerOf(fuelId) == msg.sender,
        "You're not the owner of this NFT"
    );

    // push new token to staking collection
    loaders[msg.sender].fuelIds.push(fuelId);

    // updates index reference of fuelId
    uint256 totalFuel = loaders[msg.sender].fuelIds.length;
    fuelIdIndex[msg.sender][fuelId] = totalFuel - 1;

    // inits staking block
    loaders[msg.sender].loadBlock[fuelId] = block.number;

    // add it to reference
    loaderOf[fuelId] = msg.sender;

    fuel.safeTransferFrom(address(msg.sender), address(this), fuelId);
}

function unstake(uint256 fuelId) public nonReentrant {
    // safe checks
    require(msg.sender == loaderOf[fuelId], "You are not the owner"); 
//require(ownedByThis(fuelId), "This fuel is not being loaded here!");

    // require(
    //     _loaderOf(fuelId) == address(msg.sender),
    //     "You haven't loaded this fuel here!"
    // );

    uint256 lastFuelIndex = loaders[msg.sender].fuelIds.length - 1;
    uint256 fuelIndex = fuelIdIndex[msg.sender][fuelId];

    // swap current fuelId to last position
    if (lastFuelIndex != fuelIndex) {
        uint256 lastFuelId = loaders[msg.sender].fuelIds[lastFuelIndex];

        loaders[msg.sender].fuelIds[fuelIndex] = lastFuelIndex; // Move the 
 last token to the slot of the to-delete token
        fuelIdIndex[msg.sender][lastFuelId] = fuelIndex; // Update the 
 moved token's index
    }

    // remove the last element from mapping and array
    delete fuelIdIndex[msg.sender][fuelId];
    delete loaders[msg.sender].fuelIds[lastFuelIndex];

    delete loaders[msg.sender].loadBlock[fuelId];
    

    // Transfer back to the owner
    fuel.safeTransferFrom(address(this), address(msg.sender), fuelId);
    claim(fuelId);
}

function claim(uint256 fuelId) public {
    // safe checks
    //require(ownedByThis(fuelId), "This fuel is not being loaded here!");
    require(msg.sender == loaderOf[fuelId], "You are not the owner");

    // require(
    //     _loaderOf(fuelId) == address(msg.sender),
    //     "You haven't loaded this fuel here!"
    // );

    uint256 rewardsToClaim = getPendingRewards(msg.sender, fuelId);
    energy.mintRewards(msg.sender, rewardsToClaim);

    loaders[msg.sender].loadBlock[fuelId] = block.number;
    delete loaderOf[fuelId];
}

function claimAll() public nonReentrant {
    // safe checks
    require(
        loaders[msg.sender].fuelIds.length > 0,
        "You have no fuel loaded here!"
    );

    uint256 totalFuelLoaded = totalFuelLoadedBy(msg.sender);

    for (uint256 i = 0; i < totalFuelLoaded; i++) {
        uint256 fuelId = loaders[msg.sender].fuelIds[i];
        claim(fuelId);
    }
}

function getPendingRewards(address account, uint256 fuelId) public view 
returns (uint256) {

    uint256 loadBlock = loaders[account].loadBlock[fuelId];
    uint256 blocksElapsed = block.number - loadBlock;

    return blocksElapsed * rewardsPerBlock;
}

function getAllPendingRewards() public view returns (uint256) {
    uint256 totalFuelLoaded = totalFuelLoadedBy(msg.sender);

    uint256 totalRewards = 0;
    for (uint256 i = 0; i < totalFuelLoaded; i++) {
        uint256 fuelId = loaders[msg.sender].fuelIds[i];
        totalRewards += getPendingRewards(msg.sender, fuelId);
    }

    return totalRewards;
}

function _loaderOf(uint256 fuelId) public view returns (address) {
    return loaderOf[fuelId];
}

function totalFuelLoadedBy(address account) public view returns (uint256) {
    return loaders[account].fuelIds.length;
}

function generatorAddress() public view returns (address) {
    return address(this);
}

// function ownedByThis(uint256 fuelId) public view returns (bool) {           
//     return address(fuel.ownerOf(fuelId)) == address(this);
// }

function onERC721Received(address, address, uint256, bytes memory) public 
virtual override returns (bytes4) {
    return this.onERC721Received.selector;
}
}

I do note what you say, the problem is that if you call ownedByThis separately it will return true; same with _loaderOf it will return the msg.sender address in this case. Weird thing is its just not working in the require function. — yieniggu, Aug 11 '22 at 22:02

Require statement reverting transaction everytime

1 Answers1