code-423n4 / 2022-08-frax-findings

2 stars 1 forks source link

BORROWER CAN BORROW ASSET FROM HIMSELF AND GET THE SHARES #353

Closed code423n4 closed 1 year ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairCore.sol#L739

Vulnerability details

Impact

User can borrow the asset from lender which the lender is himself. The user will get the share after he lend his asset to himself.

Proof of Concept

First the user, let say Alice, will call borrowAsset and set the address of the receiver to msg.sender. After that the receiver (Alice) will get the shares.

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairCore.sol#L739

        uint256 _borrowAmount,
        uint256 _collateralAmount,
        address _receiver
    )
        external
        isNotPastMaturity
        whenNotPaused
        nonReentrant
        isSolvent(msg.sender)
        approvedBorrower
        returns (uint256 _shares)
    {
        _addInterest();
        _updateExchangeRate();
        if (_collateralAmount > 0) {
            _addCollateral(msg.sender, _collateralAmount, msg.sender);
        }
        _shares = _borrowAsset(_borrowAmount.toUint128(), _receiver);
    }

Then Alice call deposit to lend the asset, Alice will set the address of the receiver to msg.sender

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPairCore.sol#L583

        external
        nonReentrant
        isNotPastMaturity
        whenNotPaused
        approvedLender(_receiver)
        returns (uint256 _sharesReceived)
    {
        _addInterest();
        VaultAccount memory _totalAsset = totalAsset;
        _sharesReceived = _totalAsset.toShares(_amount, false);
        _deposit(_totalAsset, _amount.toUint128(), _sharesReceived.toUint128(), _receiver);
    }

To make the interest smaller, Alice will call the repayAsset quickly for repaying her borrowing position. So Alice will pay her debt with small interest, and get the shares

Recommended Mitigation Steps

Consider to check that msg.sender is not the receiver in both deposit, mint, borrowAsset.

0xA5DF commented 2 years ago

Seems invalid, the attacker would gain nothing since he'd be the one paying the interest that he earns, plus protocol fees.

DrakeEvans commented 2 years ago

Attacker just pays himself interest, this is fine. And is helpful because it adds liquidity