search

code-423n4 / 2022-05-rubicon-findings

5 stars 2 forks source link

Lack of Access Control for offer(uint, ERC20, uint, ERC20) and insert(uint, unint) #110

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L598 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L697

Vulnerability details

Proof-of-Concept

The offer(uint, ERC20, uint, ERC20) and insert(uint, unint) should only be accessible by the keepers as per the comments. However, there is no authorisation logic or access control implemented. Therefore, anyone could call these two functions.

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L598

// Make a new offer. Takes funds from the caller into market escrow.
//
// If matching is enabled:
//     * creates new offer without putting it in
//       the sorted list.
//     * available to authorized contracts only!
//     * keepers should call insert(id,pos)
//       to put offer in the sorted list.
//
// If matching is disabled:
//     * calls expiring market's offer().
//     * available to everyone without authorization.
//     * no sorting is done.
//
function offer(
    uint256 pay_amt, //maker (ask) sell how much
    ERC20 pay_gem, //maker (ask) sell which token
    uint256 buy_amt, //taker (ask) buy how much
    ERC20 buy_gem //taker (ask) buy which token
) public override returns (uint256) {
    require(!locked, "Reentrancy attempt");

        function(uint256, ERC20, uint256, ERC20) returns (uint256) fn
     = matchingEnabled ? _offeru : super.offer;
    return fn(pay_amt, pay_gem, buy_amt, buy_gem);
}

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L697

//insert offer into the sorted list
//keepers need to use this function
function insert(
    uint256 id, //maker (ask) id
    uint256 pos //position to insert into
) public returns (bool) {
    require(!locked, "Reentrancy attempt");
    require(!isOfferSorted(id)); //make sure offers[id] is not yet sorted
    require(isActive(id)); //make sure offers[id] is active

    _hide(id); //remove offer from unsorted offers list
    _sort(id, pos); //put offer into the sorted offers list
    emit LogInsert(msg.sender, id);
    return true;
}

Impact

Following are the three offers functions that public users can use to place new orders

Per the OasisDex Documentation, which Rubicon Market based upon, the last order (offer(uint, ERC20, uint, ERC20)) method should not be used. Following is the extract from the documentation.

This method IS NOT recommended and shouldn't be used. Such an offer would not end up in the sorted list but would rather need to be inserted by a keeper at a later date. There is no guarantee that this will ever happen.

offer(uint, ERC20, uint, ERC20) and insert(uint, unint) should be reserved for authorized users (e.g. keepers) only, but the fact is that anyone could access.

The functions offer(uint,ERC20,uint,ERC20,uint) and offer(uint,ERC20,uint,ERC20,uint,bool) will trigger the matching logic, but the function offer(uint,ERC20,uint,ERC20) does not.

The function offer(uint,ERC20,uint,ERC20) allows malicious user to manipulate the orderbook in an atomic transaction by submitting a order without it being atomically matched, and then insert(uint,uint) can be used in order to manually sort the order without triggering matching.

These additional interfaces might potentially allow attacker to implement sophisticated techniques to compromise the protocol in the future. These two interfaces have been utilised by malicious users in the past to manipulate the orderbook, see https://samczsun.com/taking-undercollateralized-loans-for-fun-and-for-profit/ (Eth2Dai Section)

Recommended Mitigation Steps

Review if offer(uint, ERC20, uint, ERC20) and insert(uint, unint) is needed. If these function are not needed, it is recommended to remove these functions to reduce the attack surface of the protocol. If these functions are needed, implement the necessary access controls to ensure only authorised users can access.

bghughes commented 2 years ago

I believe this should be informational as it is a feature to allow for users to create offers outside of the sorted list. Them then inserting that offer into the list seems like appropriate functionality to me

HickupHH3 commented 2 years ago

The warden has referenced a past attack vector demonstrated by the legendary samczsun that exploited the exact same functions to manipulate prices, as well as OasisDex's documentation, which makes the issue a very strong case.

Have to therefore disagree that it's appropriate functionality. The functions mentioned by the warden should be removed to prevent potential integrations from being exploited the same way.