Skip to content

Commit

Permalink
refactor: Post-C4 Audit Cleanup (#48)
Browse files Browse the repository at this point in the history 
* chore: Remove TODO on pullFunds (c4 #10)

* chore: strict 0.8.7 (c4 #23)

* fix: Add named returns where missing (c4 #25)

* fix: `handleClaimOfRepossessed` gas optimization (c4 #66)

* fix: `getExpectedAmount` gas savings (c4 #62)

* fix: Use `!= 0` instead of `> 0` (c4 #24)

* fix: Input validation for `setAllowedSlippage` (c4 #45)

* refactor: POst-C4 Audit Cleanup

- natspec, spacing, and alignment
- factory `mapleGlobals_` check now handled by mpf
- `success_` -> `success` for non-param variable
- reordered `DebtLockerStorage`
- take latest dependencies
- `acceptNewTerms` gas savings
- `claim`, `_handleClaimOfRepossessed`, and `_handleClaim` gas savings
- `_isLiquidationActive` gas savings
- `triggerDefault` gas savings

Co-authored-by: Joao Gabriel Carvalho <[email protected]>
  • Loading branch information
@deluca-mike @JGcarv
deluca-mike and JGcarv authored Dec 13, 2021
1 parent bdc437f commit 0f705a7
Show file tree
Hide file tree
Showing 14 changed files with 101 additions and 98 deletions.
74 changes: 40 additions & 34 deletions contracts/DebtLocker.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { IERC20Like, IMapleGlobalsLike, IMapleLoanLike, IPoolLike, IPoolFactoryL

import { DebtLockerStorage } from "./DebtLockerStorage.sol";

/// @title DebtLocker interacts with Loans on behalf of PoolV1
/// @title DebtLocker interacts with Loans on behalf of PoolV1.
contract DebtLocker is IDebtLocker, DebtLockerStorage, MapleProxied {

/*****************/
Expand Down Expand Up @@ -53,32 +53,35 @@ contract DebtLocker is IDebtLocker, DebtLockerStorage, MapleProxied {
function acceptNewTerms(address refinancer_, bytes[] calldata calls_, uint256 amount_) external override whenProtocolNotPaused {
require(msg.sender == _getPoolDelegate(), "DL:ANT:NOT_PD");

IMapleLoanLike loan_ = IMapleLoanLike(_loan);
address loanAddress = _loan;

require(
(loan_.claimableFunds() + _fundsToCapture == 0) &&
(loan_.principal() == _principalRemainingAtLastClaim),
(IMapleLoanLike(loanAddress).claimableFunds() + _fundsToCapture == uint256(0)) &&
(IMapleLoanLike(loanAddress).principal() == _principalRemainingAtLastClaim),
"DL:ANT:NEED_TO_CLAIM"
);

require(amount_ == uint256(0) || ERC20Helper.transfer(loan_.fundsAsset(), address(_loan), amount_), "DL:ANT:TRANSFER_FAILED");
require(
amount_ == uint256(0) || ERC20Helper.transfer(IMapleLoanLike(loanAddress).fundsAsset(), loanAddress, amount_),
"DL:ANT:TRANSFER_FAILED"
);

loan_.acceptNewTerms(refinancer_, calls_, uint256(0));
IMapleLoanLike(loanAddress).acceptNewTerms(refinancer_, calls_, uint256(0));

// NOTE: This must be set after accepting the new terms, which affects the loan principal.
_principalRemainingAtLastClaim = loan_.principal();
_principalRemainingAtLastClaim = IMapleLoanLike(loanAddress).principal();
}

function claim() external override whenProtocolNotPaused returns (uint256[7] memory details_) {
require(msg.sender == _pool, "DL:C:NOT_POOL");

return _repossessed ? _handleClaimOfRepossessed() : _handleClaim();
return _repossessed ? _handleClaimOfRepossessed(msg.sender, _loan) : _handleClaim(msg.sender, _loan);
}

function pullFundsFromLiquidator(address token_, address destination_, uint256 amount_) external override {
require(msg.sender == _getPoolDelegate(), "DL:SA:NOT_PD");
Liquidator(_liquidator).pullFunds( token_, destination_, amount_);

Liquidator(_liquidator).pullFunds(token_, destination_, amount_);
}

function setAllowedSlippage(uint256 allowedSlippage_) external override whenProtocolNotPaused {
Expand Down Expand Up @@ -120,9 +123,11 @@ contract DebtLocker is IDebtLocker, DebtLockerStorage, MapleProxied {
function triggerDefault() external override whenProtocolNotPaused {
require(msg.sender == _pool, "DL:TD:NOT_POOL");

address loanAddress = _loan;

require(
(IMapleLoanLike(_loan).claimableFunds() == 0) &&
(IMapleLoanLike(_loan).principal() == _principalRemainingAtLastClaim),
(IMapleLoanLike(loanAddress).claimableFunds() == uint256(0)) &&
(IMapleLoanLike(loanAddress).principal() == _principalRemainingAtLastClaim),
"DL:TD:NEED_TO_CLAIM"
);

Expand All @@ -132,18 +137,18 @@ contract DebtLocker is IDebtLocker, DebtLockerStorage, MapleProxied {
// accounting is updated properly when principal is updated and there are no claimable funds.

// Repossess collateral and funds from Loan.
( uint256 collateralAssetAmount, ) = IMapleLoanLike(_loan).repossess(address(this));
( uint256 collateralAssetAmount, ) = IMapleLoanLike(loanAddress).repossess(address(this));

address collateralAsset = IMapleLoanLike(_loan).collateralAsset();
address fundsAsset = IMapleLoanLike(_loan).fundsAsset();
address collateralAsset = IMapleLoanLike(loanAddress).collateralAsset();
address fundsAsset = IMapleLoanLike(loanAddress).fundsAsset();

if (collateralAsset == fundsAsset || collateralAssetAmount == 0) return;
if (collateralAsset == fundsAsset || collateralAssetAmount == uint256(0)) return;

// Deploy Liquidator contract and transfer collateral.
require(
ERC20Helper.transfer(
collateralAsset,
_liquidator = address(new Liquidator(address(this), collateralAsset, fundsAsset, address(this), address(this),_getGlobals())),
_liquidator = address(new Liquidator(address(this), collateralAsset, fundsAsset, address(this), address(this), _getGlobals())),
collateralAssetAmount
),
"DL:TD:TRANSFER"
Expand All @@ -154,16 +159,16 @@ contract DebtLocker is IDebtLocker, DebtLockerStorage, MapleProxied {
/*** Internal Functions ***/
/**************************/

function _handleClaim() internal returns (uint256[7] memory details_) {
function _handleClaim(address pool_, address loan_) internal returns (uint256[7] memory details_) {
// Get loan state variables needed
uint256 claimableFunds = IMapleLoanLike(_loan).claimableFunds();
uint256 claimableFunds = IMapleLoanLike(loan_).claimableFunds();

require(claimableFunds > uint256(0), "DL:HC:NOTHING_TO_CLAIM");

// Send funds to pool
IMapleLoanLike(_loan).claimFunds(claimableFunds, _pool);
IMapleLoanLike(loan_).claimFunds(claimableFunds, pool_);

uint256 currentPrincipalRemaining = IMapleLoanLike(_loan).principal();
uint256 currentPrincipalRemaining = IMapleLoanLike(loan_).principal();

// Determine how much of `claimableFunds` is principal
uint256 principalPortion = _principalRemainingAtLastClaim - currentPrincipalRemaining;
Expand All @@ -186,14 +191,14 @@ contract DebtLocker is IDebtLocker, DebtLockerStorage, MapleProxied {

_fundsToCapture = uint256(0);

require(ERC20Helper.transfer(IMapleLoanLike(_loan).fundsAsset(), _pool, amountOfFundsToCapture), "DL:HC:CAPTURE_FAILED");
require(ERC20Helper.transfer(IMapleLoanLike(loan_).fundsAsset(), pool_, amountOfFundsToCapture), "DL:HC:CAPTURE_FAILED");
}
}

function _handleClaimOfRepossessed() internal returns (uint256[7] memory details_) {
function _handleClaimOfRepossessed(address pool_, address loan_) internal returns (uint256[7] memory details_) {
require(!_isLiquidationActive(), "DL:HCOR:LIQ_NOT_FINISHED");

address fundsAsset = IMapleLoanLike(_loan).fundsAsset();
address fundsAsset = IMapleLoanLike(loan_).fundsAsset();
uint256 principalToCover = _principalRemainingAtLastClaim; // Principal remaining at time of liquidation
uint256 fundsCaptured = _fundsToCapture;

Expand All @@ -205,15 +210,15 @@ contract DebtLocker is IDebtLocker, DebtLockerStorage, MapleProxied {
// If `recoveredFunds` is greater than `principalToCover`, the remaining amount is treated as interest in the context of the pool.
// If `recoveredFunds` is less than `principalToCover`, the difference is registered as a shortfall.
details_[0] = totalClaimed;
details_[1] = recoveredFunds > principalToCover ? recoveredFunds - principalToCover : 0;
details_[1] = recoveredFunds > principalToCover ? recoveredFunds - principalToCover : uint256(0);
details_[2] = fundsCaptured;
details_[5] = recoveredFunds > principalToCover ? principalToCover : recoveredFunds;
details_[6] = principalToCover > recoveredFunds ? principalToCover - recoveredFunds : 0;
details_[6] = principalToCover > recoveredFunds ? principalToCover - recoveredFunds : uint256(0);

_fundsToCapture = uint256(0);
_repossessed = false;

require(ERC20Helper.transfer(fundsAsset, _pool, totalClaimed), "DL:HCOR:TRANSFER");
require(ERC20Helper.transfer(fundsAsset, pool_, totalClaimed), "DL:HCOR:TRANSFER");
}

/**********************/
Expand All @@ -237,11 +242,10 @@ contract DebtLocker is IDebtLocker, DebtLockerStorage, MapleProxied {
}

function getExpectedAmount(uint256 swapAmount_) external view override whenProtocolNotPaused returns (uint256 returnAmount_) {
address loan = _loan;
address collateralAsset = IMapleLoanLike(loan).collateralAsset();
address fundsAsset = IMapleLoanLike(loan).fundsAsset();

address globals = _getGlobals();
address loanAddress = _loan;
address collateralAsset = IMapleLoanLike(loanAddress).collateralAsset();
address fundsAsset = IMapleLoanLike(loanAddress).fundsAsset();
address globals = _getGlobals();

uint8 collateralAssetDecimals = IERC20Like(collateralAsset).decimals();

Expand All @@ -254,7 +258,7 @@ contract DebtLocker is IDebtLocker, DebtLockerStorage, MapleProxied {
/ uint256(10) ** uint256(collateralAssetDecimals) // Convert from `fromAsset` decimal precision.
/ uint256(10_000); // Divide basis points for slippage.

uint256 minRatioAmount = swapAmount_ * _minRatio / 10 ** collateralAssetDecimals;
uint256 minRatioAmount = (swapAmount_ * _minRatio) / (uint256(10) ** collateralAssetDecimals);

return oracleAmount > minRatioAmount ? oracleAmount : minRatioAmount;
}
Expand Down Expand Up @@ -316,7 +320,9 @@ contract DebtLocker is IDebtLocker, DebtLockerStorage, MapleProxied {
}

function _isLiquidationActive() internal view returns (bool isActive_) {
return (_liquidator != address(0)) && (IERC20Like(IMapleLoanLike(_loan).collateralAsset()).balanceOf(_liquidator) != uint256(0));
address liquidatorAddress = _liquidator;

return (liquidatorAddress != address(0)) && (IERC20Like(IMapleLoanLike(_loan).collateralAsset()).balanceOf(liquidatorAddress) != uint256(0));
}

}
10 changes: 4 additions & 6 deletions contracts/DebtLockerFactory.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,14 @@ contract DebtLockerFactory is IDebtLockerFactory, MapleProxyFactory {

uint8 public constant override factoryType = uint8(1);

constructor(address mapleGlobals_) MapleProxyFactory(mapleGlobals_) {
require(mapleGlobals_ != address(0));
}
constructor(address mapleGlobals_) MapleProxyFactory(mapleGlobals_) {}

function newLocker(address loan_) external override returns (address debtLocker_) {
bytes memory arguments = abi.encode(loan_, msg.sender);

bool success_;
( success_, debtLocker_ ) = _newInstance(defaultVersion, arguments);
require(success_, "DLF:NL:FAILED");
bool success;
( success, debtLocker_ ) = _newInstance(defaultVersion, arguments);
require(success, "DLF:NL:FAILED");

emit InstanceDeployed(defaultVersion, debtLocker_, arguments);
}
Expand Down
6 changes: 3 additions & 3 deletions contracts/DebtLockerStorage.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ pragma solidity 0.8.7;
/// @title DebtLockerStorage maps the storage layout of a DebtLocker.
contract DebtLockerStorage {

bool internal _repossessed;

address internal _loan;
address internal _liquidator;
address internal _loan;
address internal _pool;

bool internal _repossessed;

uint256 internal _allowedSlippage;
uint256 internal _amountRecovered;
uint256 internal _fundsToCapture;
Expand Down
2 changes: 1 addition & 1 deletion contracts/interfaces/IDebtLocker.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ pragma solidity 0.8.7;

import { IMapleProxied } from "../../modules/maple-proxy-factory/contracts/interfaces/IMapleProxied.sol";

/// @title DebtLocker holds custody of LoanFDT tokens.
/// @title DebtLocker interacts with Loans on behalf of PoolV1.
interface IDebtLocker is IMapleProxied {

/**************/
Expand Down
2 changes: 1 addition & 1 deletion contracts/interfaces/IDebtLockerFactory.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ interface IDebtLockerFactory is IMapleProxyFactory {
function factoryType() external view returns (uint8 factoryType_);

/**
* @dev Deploys a new DebtLocker proxy instance.
* @dev Deploys a new DebtLocker proxy instance.
* @param loan_ Loan contract that corresponds to DebtLocker.
*/
function newLocker(address loan_) external returns (address debtLocker_);
Expand Down
Loading

0 comments on commit 0f705a7

Please sign in to comment.