sherlock-audit / 2022-10-union-finance-judging

4 stars 1 forks source link

bin2chen - removeAdapter/removeToken need to cancel approve #107

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

bin2chen

medium

removeAdapter/removeToken need to cancel approve

Summary

AssetManager.sol#removeToken/AssetManager.sol#removeAdapter No cancel approve , a security risk

Vulnerability Detail

when #addToken() and #addAdapter() will call approveAllTokensMax() and approveAllMarketsMax() but when remove, don't approve(0) , a security risk

Impact

A malicious moneyMarkets that has been removed still has the opportunity to transfer tokens

Code Snippet

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L433

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L389

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L396

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L440

Tool used

Manual Review

Recommendation

canel approve

    function removeToken(address tokenAddress) external override onlyAdmin {

        if (isExist) {
            supportedTokensList[index] = supportedTokensList[supportedTokensLength - 1];
            supportedTokensList.pop();
            supportedMarkets[tokenAddress] = false;
+          approveAllMarkets(tokenAddress,0);
        }
    }

    function removeAdapter(address adapterAddress) external override onlyAdmin {

        if (isExist) {
            moneyMarkets[index] = moneyMarkets[moneyMarketsLength - 1];
            moneyMarkets.pop();
+         approveAllTokens(adapterAddress,0);
        }

Duplicate of #36

dmitriia commented 1 year ago

Looks to be dup of #36