xiaoming90 - Teller Cannot Be Removed From Callback Contract

[sherlock-admin]

xiaoming90

medium

Teller Cannot Be Removed From Callback Contract

Summary

If a vulnerable Teller is being exploited by an attacker, there is no way for the owner of the Callback Contract to remove the vulnerable Teller from their Callback Contract.

Vulnerability Detail

The Callback Contract is missing the feature to remove a Teller. Once a Teller has been added to the whitelist (approvedMarkets mapping), it is not possible to remove the Teller from the whitelist.

https://github.com/sherlock-audit/2022-11-bond/blob/main/src/bases/BondBaseCallback.sol#L59

File: BondBaseCallback.sol
56:     /* ========== WHITELISTING ========== */
57: 
58:     /// @inheritdoc IBondCallback
59:     function whitelist(address teller_, uint256 id_) external override onlyOwner {
60:         // Check that the market id is a valid, live market on the aggregator
61:         try _aggregator.isLive(id_) returns (bool live) {
62:             if (!live) revert Callback_MarketNotSupported(id_);
63:         } catch {
64:             revert Callback_MarketNotSupported(id_);
65:         }
66: 
67:         // Check that the provided teller is the teller for the market ID on the stored aggregator
68:         // We could pull the teller from the aggregator, but requiring the teller to be passed in
69:         // is more explicit about which contract is being whitelisted
70:         if (teller_ != address(_aggregator.getTeller(id_))) revert Callback_TellerMismatch();
71: 
72:         approvedMarkets[teller_][id_] = true;
73:     }

Impact

In the event that a whitelisted Teller is found to be vulnerable and has been actively exploited by an attacker in the wild, the owner of the Callback Contract needs to mitigate the issue swiftly by removing the vulnerable Teller from the Callback Contract to stop it from draining the asset within the Callback Contract. However, the mitigation effort will be hindered by the fact there is no way to remove a Teller within the Callback Contract once it has been whitelisted. Thus, it might not be possible to stop the attacker from exploiting the vulnerable Teller to drain assets within the Callback Contract. The Callback Contract owners would need to find a workaround to block the attack, which will introduce an unnecessary delay to the recovery process where every second counts.

Additionally, if the owner accidentally whitelisted the wrong Teller, there is no way to remove it.

Code Snippet

https://github.com/sherlock-audit/2022-11-bond/blob/main/src/bases/BondBaseCallback.sol#L59

Tool used

Manual Review

Recommendation

Consider implementing an additional function to allow the removal of a Teller from the whitelist (approvedMarkets mapping), so that a vulnerable Teller can be removed swiftly if needed.

function removeFromWhitelist(address teller_, uint256 id_) external override onlyOwner {
    approvedMarkets[teller_][id_] = false;
}

Note: Although the owner of the Callback Contract can DOS its own market by abusing the removeFromWhitelist function, no sensible owner would do so.

[Evert0x]

Message from sponsor

---

Agree with this issue. We implemented a blacklist() function on the BondBaseCallback.sol contract to allow removing a teller and market ID combination from using the callback.

[xiaoming9090]

Fixed in https://github.com/Bond-Protocol/bonds/commit/368e6c5b120d9fc44c59cc21d33ee51728728067