Debt DAO contest - datapunk's results

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/6987988fe39901cad9a8e5ebb2c6aa719590873d/contracts/utils/LineLib.sol#L59

Vulnerability details

Impact

receiveTokenOrETH() receives funds. However, there is code path within the function that does not require Ether. Ether passed to the function, when the non-Ether code path is taken, is locked in the contract forever, and the sender gets nothing extra in return for it.

Proof of Concept

If ETH is provided for a ERC20 credit line, it would not be accounted for, thus lost forever to the user. msg.value shouldn’t be provided here:

if(token != Denominations.ETH) { // ERC20 @audit add require(msg.value==0);
    IERC20(token).safeTransferFrom(sender, address(this), amount);
} 

Tools Used

VSCode

Recommended Mitigation Steps

Add a require(0 == msg.value) for the above three conditions.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/6987988fe39901cad9a8e5ebb2c6aa719590873d/contracts/utils/LineLib.sol#L48

Vulnerability details

impact

The use of payable.transfer() is heavily frowned upon because it can lead to the locking of funds. The transfer() call requires that the recipient has a payable callback, only provides 2300 gas for its operation. This means the following cases can cause the transfer to fail:

The contract does not have a payable callback The contract’s payable callback spends more than 2300 gas (which is only enough to emit something) The contract is called through a proxy which itself uses up the 2300 gas If a user falls into one of the above categories, they’ll be unable to receive funds from the vault in a migration wrapper. Inaccessible funds means loss of funds, which is Medium severity.

Proof of Concept

payable(receiver).transfer(amount); in https://github.com/debtdao/Line-of-Credit/blob/6987988fe39901cad9a8e5ebb2c6aa719590873d/contracts/utils/LineLib.sol#L48

Recommended Mitigation Steps

Use address.call{value:x}() instead.