code-423n4 / 2021-12-maple-findings

0 stars 0 forks source link

Reuse arithmetic results can save gas #66

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

https://github.com/maple-labs/debt-locker/blob/81f55907db7b23d27e839b9f9f73282184ed4744/contracts/DebtLocker.sol#L205-L215

function _handleClaimOfRepossessed() internal returns (uint256[7] memory details_) {
    ...
    details_[0] = recoveredFunds + fundsCaptured;
    details_[1] = recoveredFunds > principalToCover ? recoveredFunds - principalToCover : 0;
    details_[2] = fundsCaptured;
    details_[5] = recoveredFunds > principalToCover ? principalToCover : recoveredFunds;
    details_[6] = principalToCover > recoveredFunds ? principalToCover - recoveredFunds : 0;

    _fundsToCapture = uint256(0);
    _repossessed    = false;

    require(ERC20Helper.transfer(fundsAsset, _pool, recoveredFunds + fundsCaptured), "DL:HCOR:TRANSFER");
}

recoveredFunds + fundsCaptured at L215 is calculated before at L205, since it's a checked arithmetic operation with two memory variables, resue the result instead of doing the arithmetic operation again can save gas.

Recommendation

Change to:

require(ERC20Helper.transfer(fundsAsset, _pool, details_[0]), "DL:HCOR:TRANSFER");

lucas-manuel commented 2 years ago

Thanks will add

deluca-mike commented 2 years ago

Actually, the MLOAD for details_[0] is more expensive than the DUPs and ADD of another recoveredFunds + fundsCaptured. Instead, we will just cache the result on the stack before L205, and use it twice.

pauliax commented 2 years ago

As per the sponsor's comment, the mitigation suggested by the warden is not cheaper but anyway it helped to find an optimization so I am leaving this issue as valid.