Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing a check for minimum sell amount at make function #1167

Open
code423n4 opened this issue Apr 13, 2023 · 3 comments
Open

Missing a check for minimum sell amount at make function #1167

code423n4 opened this issue Apr 13, 2023 · 3 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-07 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/RubiconMarket.sol#L835
https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/RubiconMarket.sol#L511
https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/RubiconMarket.sol#L511-L562

Vulnerability details

Summary

https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/RubiconMarket.sol#L835
There is a check for _dust amount inside offer function at L835
but not inside the other offer function being called by make function at L511

Impact

make function will not avoid dust offers

Proof of Concept

https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/RubiconMarket.sol#L511-L562
There is no check for _dust inside offer function, the minimum sell amount should be set for a token to avoid dust offers as the concept apply.

    function offer(
        uint256 pay_amt,
        ERC20 pay_gem,
        uint256 buy_amt,
        ERC20 buy_gem,
        address owner,
        address recipient
    ) public virtual can_offer synchronized returns (uint256 id) {
        require(uint128(pay_amt) == pay_amt);
        require(uint128(buy_amt) == buy_amt);
        require(pay_amt > 0);
        require(pay_gem != ERC20(address(0))); /// @dev Note, modified from: require(pay_gem != ERC20(0x0)) which compiles in 0.7.6
        require(buy_amt > 0);
        require(buy_gem != ERC20(address(0))); /// @dev Note, modified from: require(buy_gem != ERC20(0x0)) which compiles in 0.7.6
        require(pay_gem != buy_gem);


        OfferInfo memory info;
        info.pay_amt = pay_amt;
        info.pay_gem = pay_gem;
        info.buy_amt = buy_amt;
        info.buy_gem = buy_gem;
        info.recipient = recipient;
        info.owner = owner;
        info.timestamp = uint64(block.timestamp);
        id = _next_id();
        offers[id] = info;


        require(pay_gem.transferFrom(msg.sender, address(this), pay_amt));


        emit LogItemUpdate(id);


        /// emit LogMake(
        ///     bytes32(id),
        ///     keccak256(abi.encodePacked(pay_gem, buy_gem)),
        ///     msg.sender,
        ///     pay_gem,
        ///     buy_gem,
        ///     uint128(pay_amt),
        ///     uint128(buy_amt),
        ///     uint64(block.timestamp)
        /// );


        emit emitOffer(
            bytes32(id),
            keccak256(abi.encodePacked(pay_gem, buy_gem)),
            msg.sender,
            pay_gem,
            buy_gem,
            uint128(pay_amt),
            uint128(buy_amt)
        );
    }

as you see it is applied in the other offer function as well at L835

   require(_dust[address(pay_gem)] <= pay_amt);

Tools Used

Manual Review

Recommended Mitigation Steps

Add the check condition:

   require(_dust[address(pay_gem)] <= pay_amt);

to offer function that called by make function

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Apr 13, 2023
code423n4 added a commit that referenced this issue Apr 13, 2023
@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label May 2, 2023
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as primary issue

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label May 3, 2023
@c4-sponsor
Copy link

daoio marked the issue as sponsor confirmed

@c4-judge
Copy link
Contributor

HickupHH3 marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label May 25, 2023
@C4-Staff C4-Staff added the M-07 label Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-07 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

5 participants