Ignored return value from "IERC20.transferFrom()"

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/StableVault.sol#L46
https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Lock.sol#L72
https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Lock.sol#L90
https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L216

Vulnerability details

Impact

The return value from IERC20.transferFrom in "StableVault" was not checked, a malicious actor could first deposit a number of tokens without actually having it and then subsequently withdraw that amount of tokens from the "StableVault"

Proof of Concept

first, by calling the function

function deposit(address _token, uint256 _amount) public {
        require(allowed[_token], "Token not listed");
        IERC20(_token).transferFrom(_msgSender(), address(this), _amount);
        IERC20Mintable(stable).mintFor(
            _msgSender(),
            _amount*(10**(18-IERC20Mintable(_token).decimals()))
        );
    }

with the address of a token in the allowed mapping, the contract mints a "StableToken" with the number of tokens passed to the function
mint function

Then a call to the "withdraw" function will send tokens to the caller

function withdraw(address _token, uint256 _amount) external returns (uint256 _output) {
        IERC20Mintable(stable).burnFrom(_msgSender(), _amount);
        _output = _amount/10**(18-IERC20Mintable(_token).decimals());
        IERC20(_token).transfer(
            _msgSender(),
            _output
        );
    }

Tools Used

Brownie, Slither

Recommended Mitigation Steps

The above-mentioned attack can be mitigated by changing the line

IERC20(_token).transferFrom(_msgSender(), address(this), _amount);

to

require(IERC20(_token).transferFrom(_msgSender(), address(this), _amount)
    "Insufficient Funds");

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Out of scope

[GalloDaSballo]

Valid but OOS per Readme