Assessment reports>WOOFi Swap>Discussion>The WooPPV2 contract doesn't need to be an admin

The WooPPV2 contract doesn't need to be an admin

When the WooPPV2 contract makes a swap, it calls postPrice on the WooracleV2_2 to post the new price for future swaps:

IWooracleV2_2(wooracle).postPrice(baseToken, uint128(newPrice));

Here is the implementation of postPrice:

function postPrice(address _base, uint128 _price) external onlyAdmin {
    infos[_base].price = _price;
    if (msg.sender != wooPP) {
        timestamp = block.timestamp;
    }
}

Since postPrice is onlyAdmin, this means the wooPP address must be set as an admin on the oracle contract. However, this means that the wooPP can call any other admin function, including the fallback function.

There is currently no way for an unprivileged caller to cause WooPPV2 to call the oracle in an unexpected manner without it also reverting. But, since the fallback function accepts many function selectors, a future version of either contract could introduce a potential negative interaction where the pool calls an external contract using a user-controlled address, for instance to transfer an unknown ERC20 token or to make a callback call, and an attacker unexpectedly passes in the address of the oracle instead of the token or receiver address.

Since postPrice already checks if the caller is wooPP, it wouldn't cost any more gas to repeat the body of the onlyAdmin check in the function with an extra case for wooPP. Alternatively, another function can be added that only allows wooPP to call it, that posts the price without resetting the timestamp. Either of these two strategies would make it so that wooPP cannot call any of the other onlyAdmin functions.

This issue has been acknowledged by WOOFI, and the fix has been implemented in commit ea79a0d

Zellic © 2024Back to top ↑