Hey Alex! I’ve just done a quick review with the findings below, I’m happy to do another run through afterwards if you’d like.
1. Is the smart contract secure?
Funds can potentially be locked or caused to be liquidated by sending different aTokens to the contract
The functions deposited, owed, canBorrow and canRepay are used to calculate the amount of MATIC the contract has deposited/borrowed from Aave - and so how much it can safely deposit/withdraw. This is done by querying the total value of assets deposits/borrowed and then calculating the equivalent value number of MATIC.
The issue arises because it doesn’t check that these assets are MATIC tokens. This leads to a situation where a malicious party can deposit a different token into Aave of behalf of the contract, giving it (say) aUSDC and the contract will think that it has deposited more MATIC then it has.
In the case of canRepay the contract can be made to think it can withdraw more MATIC than it owns, resulting in reverts when repaying debt. In the case of canBorrow, the contract will use the borrowing power granted by it’s deposit of aUSDC to borrow MATIC. The contract then takes up a directional position on this pair of assets and should a large price change occur it may experience a liquidation.
There’s no incentive for someone to send aTokens to this contract however this issue can be easily fixed. I would recommend instead directly querying the contract’s balance of aMATIC and Debt Tokens - Developers. This also removes the need to convert everything to a value denominated in MATIC when using different tokens.
Contract borrows such that it is below MIN_HEALTH
In the canBorrow function we don’t borrow such that we stop at the health factor MIN_HEALTH so the contract will counter-intuitively fall below that number. In the snippet below if healthFactor = MIN_HEALTH+1, then the contract will use up 95% of the remaining headroom which will take the contract below the stated minimum.
function canBorrow() public view returns (uint256) {
(
uint256 totalCollateralETH,
uint256 totalDebtETH,
uint256 availableBorrowsETH,
uint256 currentLiquidationThreshold,
uint256 ltv,
uint256 healthFactor
) = ILendingPool(LENDING_POOL).getUserAccountData(address(this));
// We borrow only if we are above MIN_HEALTH
if(healthFactor > MIN_HEALTH) {
// 95% of converted to want from Eth
uint256 maxValue = PRECISION.mul(availableBorrowsETH).mul(95).div(100).div(PRECISION);
// 18 decimals
return PRECISION.mul(maxValue).mul(10**18).div(getRate()).div(PRECISION);
}
return 0;
}
While not a security issue in and of itself, it being named as MIN_HEALTH may lead to it being relied upon such that it causes funds to be locked under some conditions. We can instead calculate the borrow amount which take the contract to the desired health factor by rearranging the below formula for amountInEth:
(totalCollateralInETH * LTV) / (totalDebtInETH + amountInEth) == MIN_HEALTH
2. Can you help me write a better way to partially withdraw without wasting all the gas?
To be able to safely withdraw amount of MATIC we need to make sure that we have (ignoring precision) amount / ltv of headroom of collateral to withdraw before we hit our limit on the health factor. Note this is the LTV of the asset, not the user so you’ll have to pull it from Aave separately, see LendingPool - Developers
We may need to do a few iterations of paying back loans to get to the point where we can withdraw this much MATIC, the pseudocode for when we can perform this final withdrawal is
function canWithdrawFunds (uint256 amount) internal returns (bool) {
/* Pull data from Aave */
if (totalDebtInETH == 0) return true;
uint256 amountInETH = getEthValue(amount)
return (totalCollateralInETH - amountInETH) * LTV / totalDebtInETH > MIN_HEALTH;
}
Relevant Aave code: protocol-v2/GenericLogic.sol at baeb455fad42d3160d571bd8d3a795948b72dd85 · aave/protocol-v2 · GitHub
Posting the relevant function here:
function _divestFromAAVE (uint256 amount) internal {
require(amount <= getTotalValue(), "Cannot withdraw more than totalValue");
// Loop to repay debt until we have enough headroom to withdraw safely
while (!canWithdrawFunds(amount)) {
_withdrawStepFromAAVE(canRepay());
}
// Withdraw the desired amount
ILendingPool(LENDING_POOL).withdraw(want, amount, address(this));
}
This could be optimised further as the final iteration of _withdrawStepFromAAVE still repays the maximum amount of debt possible rather than just what’s needed to allow the withdrawal.
Misc notes
PRECISION does not increase precision of calculations
In quite a few cases we have a calculation which is in the format shown below
function fromSharesToWithdrawal(uint256 amount) public view returns (uint256){
return PRECISION.mul(amount).mul(getTotalValue()).div(totalSupply()).div(PRECISION);
}
This doesn’t look to me to increase precision except in the case where you would have a poor order of operations in the central calculation (which I can’t see in the codebase).
Unit testing against Aave
For testing against Aave, I’d recommend just forking Kovan and running tests against that when it’s not possible to mock it locally. Hardhat allows forking quite easily:
[https://]hardhat[.]org/guides/mainnet-forking.html