Cache external call results can save gas

[code423n4]

Handle

WatchPug

Vulnerability details

Every call to an external contract costs a decent amount of gas. For optimization of gas usage, external call results should be cached if they are being used for more than one time.

For example:

https://github.com/code-423n4/2021-12-mellow/blob/6679e2dd118b33481ee81ad013ece4ea723327b5/mellow-vaults/contracts/AaveVault.sol#L101-L109

function _allowTokenIfNecessary(address token) internal {
    if (IERC20(token).allowance(address(this), address(_lendingPool())) < type(uint256).max / 2) {
        IERC20(token).approve(address(_lendingPool()), type(uint256).max);
    }
}

function _lendingPool() internal view returns (ILendingPool) {
    return IAaveVaultGovernance(address(_vaultGovernance)).delayedProtocolParams().lendingPool;
}

Considering that _lendingPool() is a internal call that includes a storage read of _vaultGovernance and an external call of IAaveVaultGovernance.delayedProtocolParams(). Cache the result of _lendingPool() in the stack can save some gas.

Recommendation

Change to:

function _allowTokenIfNecessary(address token) internal {
    address lendingPool = address(_lendingPool());
    if (IERC20(token).allowance(address(this), lendingPool) < type(uint256).max / 2) {
        IERC20(token).approve(lendingPool, type(uint256).max);
    }
}

[MihanixA]

We need to check if delayedProtocolParams have changed by the Governance

[0xleastwood]

agree with warden here, _lendingPool return value can be cached once to utilise only a single external call from within _allowTokenIfNecessary

[MihanixA]

Oh, you mean caching _landingPool inside this function. Ok, lgtm.