ERC20 approval confusion

Question

I have a smart contract which looks like this.

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/access/AccessControl.sol";
import "hardhat/console.sol";

contract SocksToken is ERC20, AccessControl {
    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
    bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");
    bytes32 public constant REWARD_SETTER_ROLE = keccak256("REWARD_SETTER_ROLE");

    IERC20 public usdt;
    IERC20 public usdc;

    mapping(address => uint256) public rewards;
    mapping(address => uint256) public lastClaim;

    uint256 public stakingRewardPercentage = 1000; // Staking reward percentage in basis points (1 basis point = 0.01%)

    struct StakingData {
        uint256 amount;
        uint256 timestamp;
    }
    mapping(address => StakingData) public stakingData;

    constructor(address usdtAddress, address usdcAddress) ERC20("Socks Token", "SOCKS") {
        usdt = IERC20(usdtAddress);
        usdc = IERC20(usdcAddress);
        _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
    }

    function mint(address to, uint256 amount) external onlyRole(MINTER_ROLE) {
        _mint(to, amount);
    }

    function burn(address from, uint256 amount) external onlyRole(BURNER_ROLE) {
        _burn(from, amount);
    }

    function buy(uint256 clayAmount, bool useUSDT) external {
        uint256 buyRate = getBuyRate(useUSDT);
        console.log("buy rate", buyRate);
        uint256 cost = clayAmount * buyRate;
        console.log("cost", cost);
        if (useUSDT) {
            usdt.transferFrom(msg.sender, address(this), cost);
        } else {
            usdc.transferFrom(msg.sender, address(this), cost);
        }
        _mint(msg.sender, clayAmount);
        console.log("supply after buy", totalSupply());
        console.log("bal after buy usdc:", usdc.balanceOf(address(this)));
        console.log("bal after buy usdt", usdt.balanceOf(address(this)));
    }

    function sell(uint256 clayAmount, bool receiveUSDT) external {
        require((usdt.balanceOf(address(this)) * 10**6) // USDT has 6 decimal places, so we multiply by 10^12 to bring it up to 18 decimal places
            + (usdc.balanceOf(address(this)) * 10**18) > 0, "No Money in contract");
        uint256 ex = getSellRate();  // sell rate is always supply vs USD pool 
        _burn(msg.sender, clayAmount);
         
        console.log("sell rate", ex);
        uint256 proceeds = clayAmount * ex;
        uint256 proceedsAdjusted;
        console.log("bal usdc", usdc.balanceOf(address(this)));
        console.log("bal usdt", usdt.balanceOf(address(this)));
        console.log("total supply", totalSupply());
        if (receiveUSDT) {
            proceedsAdjusted = proceeds / 10**6;  // adjust from 18 to 6 decimal places
            require(usdt.balanceOf(address(this)) >= proceedsAdjusted, "Not enough USDT in the contract");
            usdt.transfer(msg.sender, proceedsAdjusted);
        } else {
            require(usdc.balanceOf(address(this)) >= proceeds, "Not enough USDC in the contract");
            usdc.transfer(msg.sender, proceeds);
        }
    }


    function setReward(address user, uint256 amount) external onlyRole(REWARD_SETTER_ROLE) {
        rewards[user] = amount;
    }

    function claimReward() external {
        require(block.timestamp - lastClaim[msg.sender] > 30 days, "Can only claim reward once a month");
        uint256 reward = rewards[msg.sender];
        require(reward > 0, "No reward to claim");
        _mint(msg.sender, reward);
        lastClaim[msg.sender] = block.timestamp;
    }

    function stake(uint256 amount) external {
        approve(msg.sender, amount);
        console.log("token alllowance", allowance(address(this), msg.sender));
        // Transfer the tokens to this contract
        console.log("total supply before", balanceOf(address(this)) );
        transferFrom(msg.sender, address(this), amount);
        console.log("total supply aftre", balanceOf(address(this)) );
        // Calculate the reward using the staking reward percentage
        uint256 reward = amount * stakingRewardPercentage / 10000;

        // Update the staking data
        stakingData[msg.sender] = StakingData({
            amount: stakingData[msg.sender].amount + amount,
            timestamp: block.timestamp
        });

        // Add the reward to the user's rewards
        rewards[msg.sender] += reward;
    }

    function setStakingRewardPercentage(uint256 newPercentage) external onlyRole(DEFAULT_ADMIN_ROLE) {
        require(newPercentage <= 10000, "Percentage cannot be more than 100%");
        stakingRewardPercentage = newPercentage;
    }

    function unstake(uint256 amount) external {
        require(stakingData[msg.sender].amount >= amount, "Not enough tokens staked");
        stakingData[msg.sender].amount -= amount;
        uint256 rewardReduction = (amount * rewards[msg.sender]) / stakingData[msg.sender].amount;
        rewards[msg.sender] -= rewardReduction;
        _transfer(address(this), msg.sender, amount);
    }

    function getSellRate() public view returns(uint){
        uint256 totalReserves = (usdt.balanceOf(address(this)) * 10**12) // USDT has 6 decimal places, so we multiply by 10^12 to bring it up to 18 decimal places
            + usdc.balanceOf(address(this)); // USDC has 18 decimal places, so no adjustment is necessary

        uint256 calculatedRate = totalReserves / totalSupply();
        return calculatedRate;
    }

    function getBuyRate(bool USDT) public view returns(uint){
        uint256 totalSupply = totalSupply();
        
        if (totalSupply == 0) {
            // If totalSupply is 0, return 1 USDT in wei
            if(USDT){
                return 1 * 10**6; // USDT has 6 decimal places
            }else{
                return 1 * 10**18;
            }
        }

        uint256 totalReserves = (usdt.balanceOf(address(this)) * 10**12) // USDT has 6 decimal places, so we multiply by 10^12 to bring it up to 18 decimal places
            + usdc.balanceOf(address(this)); // USDC has 18 decimal places, so no adjustment is necessary
        uint256 calculatedRate = totalReserves / totalSupply;
        uint256 buyRate = (calculatedRate / 100) * 102;
        return buyRate;
    }

}

As you can see in the stake function I call the approve() function with msg.sender as a variables. I understand this is not best practice, however as far as I can work out this is the only way for my unit tests to work. Without this in the function I get an allowance exceeded error. The tests are here.

 describe("Staking", function () {
    it("Should allow users to stake tokens", async function () {
      await socks.grantRole(socks.MINTER_ROLE(), owner.address);
      await socks.mint(addr1.address, 100000);
      await socks.setStakingRewardPercentage(1000); // Set staking reward to 10%

      // Add the approval before staking
      await socks.approve(addr1.address, ethers.utils.parseEther("1000"));

      await socks.connect(addr1).stake(100);
      const stakingData = await socks.stakingData(addr1.address);
      expect(stakingData.amount).to.equal(100);
    });

    it("Should not allow users to unstake more tokens than they have staked", async function () {
      await socks.grantRole(socks.MINTER_ROLE(), owner.address);
      await socks.mint(owner.address, 100000);
      await socks.setStakingRewardPercentage(1000); // Set staking reward to 10%

      // Add the approval before staking
      await socks.connect(owner).approve(socks.address, ethers.utils.parseEther("1000"));
      

      await socks.connect(owner).stake(100);
      await expect(socks.connect(owner).unstake(200)).to.be.revertedWith('Not enough tokens staked');
    });

    it("Should allow users to unstake tokens", async function () {
      await socks.grantRole(socks.MINTER_ROLE(), owner.address);
      await socks.mint(owner.address, 100000);
      await socks.setStakingRewardPercentage(1000); // Set staking reward to 10%

      // Add the approval before staking
      await socks.connect(owner).approve(socks.address, ethers.utils.parseEther("1000"));

      await socks.connect(owner).stake(100);
      await socks.connect(owner).unstake(50);
      const stakingData = await socks.stakingData(owner.address);
      expect(stakingData.amount).to.equal(50);
    });
});

Is the acceptable in the contract, am I exposing some risk factors?

Answer 1

I'm not sure how approve(msg.sender, amount); in the stake function makes it work. Because it approves the msg.sender tokens to be spent by msg.sender, which isn't what we need. We need the spender to be address(this).

So I would actually try rewriting it to approve(address(this), amount);. Otherwise remove the approve there and try this, but I haven't tested if it works:

    it("Should allow users to stake tokens", async function () {
      await socks.grantRole(socks.MINTER_ROLE(), owner.address);
      await socks.mint(addr1.address, 100000);
      await socks.setStakingRewardPercentage(1000); // Set staking reward to 10%

      // Add the approval before staking
      await socks.connect(addr1).approve(socks.address, ethers.utils.parseEther("1000"));  // Basically changed this line

      await socks.connect(addr1).stake(100);
      const stakingData = await socks.stakingData(addr1.address);
      expect(stakingData.amount).to.equal(100);
    });