sherlock-audit / 2022-09-notional-judging

4 stars 2 forks source link

xiaoming90 - Approve Returned Value Not Validated #60

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

xiaoming90

medium

Approve Returned Value Not Validated

Summary

The returned value of an approve() call is not validated.

Vulnerability Detail

During vault initialization, it will attempt to perform tokenAddress.approve to allow Notional to pull the lend underlying currency at Line 100. However, it does not check the success return value. Some tokens do not revert if the approval failed but return false instead.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/CrossCurrencyfCashVault.sol#L79

File: CrossCurrencyfCashVault.sol
079:     function initialize(
080:         string memory name_,
081:         uint16 borrowCurrencyId_,
082:         uint16 lendCurrencyId_,
083:         uint64 settlementSlippageLimit_
084:     ) external initializer {
085:         __INIT_VAULT(name_, borrowCurrencyId_);
086: 
087:         LEND_CURRENCY_ID = lendCurrencyId_;
088:         (
089:             Token memory assetToken,
090:             Token memory underlyingToken,
091:             /* ETHRate memory ethRate */,
092:             /* AssetRateParameters memory assetRate */
093:         ) = NOTIONAL.getCurrencyAndRates(lendCurrencyId_);
094: 
095:         IERC20 tokenAddress = assetToken.tokenType == TokenType.NonMintable ?
096:             IERC20(assetToken.tokenAddress) : IERC20(underlyingToken.tokenAddress);
097:         LEND_UNDERLYING_TOKEN = tokenAddress;
098: 
099:         // Allow Notional to pull the lend underlying currency
100:         tokenAddress.approve(address(NOTIONAL), type(uint256).max);
101: 
102:         // This value cannot be greater than 1e18
103:         require(settlementSlippageLimit_ < SETTLEMENT_SLIPPAGE_PRECISION);
104:         settlementSlippageLimit = settlementSlippageLimit_;
105:     }

Impact

Tokens that don't actually perform the approve and return false might be counted as a correct approve. Thus, the function might proceed with the execution and assume that there is sufficient allowance to work with. This might result in a revert at the later stage of the execution when the code notice that there is insufficient allowance during transfer causing features within the vault to stop working.

Code Snippet

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/CrossCurrencyfCashVault.sol#L79

Tool used

Manual Review

Recommendation

Check the return value of ERC20 approve operation to validate that they were successfully completed.