Fraxlend (Frax Finance) contest - 0x52's results

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L409-L497

Vulnerability details

Impact

Borrower charged penalty rate for time before maturity

Proof of Concept

if (_isPastMaturity()) {
    _newRate = uint64(penaltyRate);
} else {

_currentRateInfo.ratePerSec = _newRate;
_currentRateInfo.lastTimestamp = uint64(block.timestamp);
_currentRateInfo.lastBlock = uint64(block.number);

// Calculate interest accrued
_interestEarned = (_deltaTime * _totalBorrow.amount * _currentRateInfo.ratePerSec) / 1e18;

When _addInterest is called after maturity, the interest rate is set to the penalty rate. This rate is then applied for all the time between the last _addInterest call. This means that the first time that _addInterest is called after maturity, it will back charge the borrowers penalty interest for time before maturity up until when it was last updated. This is unfair to borrower who should only be charged penalty interest for time after maturity.

Tools Used

Recommended Mitigation Steps

For the first call after maturity, the interest rate should be calculated as normal and applied up to maturity. Then the penaltyRate should be used from maturity until current time.

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPair.sol#L136-L138 https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPair.sol#L140-L142

Vulnerability details

Impact

FraxlendPair.sol is not EIP-4626 compliant, variation from the standard could break composability and potentially lead to loss of funds

Proof of Concept

According to EIP-4626 method specifications (https://eips.ethereum.org/EIPS/eip-4626)

For maxDeposit:

MUST factor in both global and user-specific limits, like if deposits are entirely disabled (even temporarily) it MUST return 0.

For maxMint:

MUST factor in both global and user-specific limits, like if mints are entirely disabled (even temporarily) it MUST return 0.

When FraxlendPair.sol is paused, deposit and mint are both disabled. This means that maxMint and maxDeposit should return 0 when the contract is paused.

The current implementations of maxMint and maxDeposit do not follow this specification:

function maxDeposit(address) external pure returns (uint256) {
    return type(uint128).max;
}

function maxMint(address) external pure returns (uint256) {
    return type(uint128).max;
}

No matter the state of the contract they always return uint128.max, but they should return 0 when the contract is paused.

Tools Used

Recommended Mitigation Steps

maxDeposit and maxMint should be updated to return 0 when contract is paused. Use of the whenNotPaused modifier is not appropriate because that would cause a revert and maxDeposit and maxMint should never revert according to EIP-4626

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L911-L942 https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairCore.sol#L950-L1032

Vulnerability details

Impact

Lender blocks liquidations

Proof of Concept

FraxlendPair.sol#setApprovedLenders can be called by any whitelisted lender to both revoke and add approved lenders to the whitelist. A malicious lender could remove every other whitelisted lender from the pair then refuse to liquidate the position themselves, making it impossible to liquidate.

Exploiting this is straight forward. If the lending market is only whitelisted for lenders, the malicious lender could withdraw their lending position, deposit collateral and borrow everything else in the pool then remove all other lenders from the pool. Something like this could be used to open a high leverage zero risk position.

If the market has whitelist borrowers, the lender could easily collude with a borrower to pull off the attack.

Tools Used

Recommended Mitigation Steps

I don't see why FraxlendPairCore.sol#liquidate and liquidateClean can only be called by whitelist lenders. It is in the best interest of both parties if anyone can liquidate a position even in a whitelisted market. If it truly is desirable that only whitelist lenders are able to liquidate, then FraxlendPair.sol#setApprovedLenders should be modified to only allow lenders to add new lenders rather than remove current lenders.