Revert on ERC20.transferFrom

Question

My code is reverting when testing a function that contains a transferFrom. The revert is isolated to this line, when commented out, it runs fine.

My (wrong) hypotheses so far:

1. Issue approving tokens (wrong from address, or wrong Loan contract address?)

2. Passing wrong this.Token.address to createLoan

Any ideas on what else could be the issue?

Here is my contract Loan.sol

pragma solidity ^0.5.0;

import "openzeppelin-solidity/contracts/math/SafeMath.sol";
import "openzeppelin-solidity/contracts/token/ERC20/ERC20.sol";
import "openzeppelin-solidity/contracts/ownership/Ownable.sol";
import "./DSmath.sol";

contract Loan is Ownable, DSMath {

...
function createLoan
    (
        uint _loanAmount,
        uint _collateralAmount,
        address _collateralAddress
    ) 
    external {
        require(loaneeToDebt[msg.sender] == 0, "User already owes tokens");
        require
        (
            isCollateralized(_loanAmount, _collateralAmount, _collateralAddress),
            "Collateral posted is insufficient to receive a loan"
        );
        require(tokenPrices[_collateralAddress] != 0, "Collateral token not registered to system");     

        ERC20(_collateralAddress).transferFrom(msg.sender, address(this), _collateralAmount); //REVERTS HERE

        loaneeToDebt[msg.sender] = _collateralAmount;

    }

Which I am testing like this in Loan.test.js:

// Loan.test.js
const {BN, expectEvent, shouldFail, constants} = require("openzeppelin-test-helpers");
const Loan = artifacts.require("Loan");
const ERC20Mock = artifacts.require("ERC20Mock")

contract("Loan", function ([_, contractOwner, user]) {

    const initialSupply = new BN(1).mul(new BN(10).pow(new BN(28)))
    beforeEach(async function () {
        this.Loan = await Loan.new({from: contractOwner});
        this.Token = await ERC20Mock.new(user, initialSupply)
    });

    describe("#createLoan", function () {
        const collateralAmount = new BN(5).mul(new BN(10).pow(new BN(27)))
        const loanAmount = new BN(1).mul(new BN(10).pow(new BN(24)))
        const tokenPrice = new BN(1)
        beforeEach(async function () {
            await this.Loan.setTokenPrice(this.Token.address, tokenPrice, {from: contractOwner});
        });

        it("should revert if the user has an outstanding loan", async function () {
            await this.Token.approve(this.Loan.address, collateralAmount, {from: user}); // APPROVAL
            await this.Loan.createLoan(loanAmount, collateralAmount, this.Token.address, {from: user}) // REVERTS HERE
            shouldFail.reverting(this.Loan.createLoan(loanAmount, collateralAmount, this.Token.address, {from: user});
        });
    });
});

With ERC20Mock:

pragma solidity ^0.5.0;
import "openzeppelin-solidity/contracts/token/ERC20/ERC20.sol";

contract ERC20Mock is ERC20 {
    constructor (address initialAccount, uint256 initialBalance) public {
        _mint(initialAccount, initialBalance);
    }

    function mint(address account, uint256 amount) public {
        _mint(account, amount);
    }

    function burn(address account, uint256 amount) public {
        _burn(account, amount);
    }

    function burnFrom(address account, uint256 amount) public {
        _burnFrom(account, amount);
    }
}

Answer 1

To be able to do a transferFrom, you need before to approve the user.

  function approve(address spender, uint256 value) external returns (bool);

In your case the loan contract should be approve by the message sender for the same amount as _collateralAmount.

You can also check how much allowance one have with the function below:

  function allowance(address owner, address spender) external view returns (uint256);

It would be worth to put a require at the begining of createLoan() to check that the allowance is sufficient.