code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L393

Vulnerability details

Impact

Witch._payInk() works only when liquidatorCut > 0 and auction.auctioneer wouldn't get paid when liquidatorCut = 0 and auctioneerCut > 0.

Proof of Concept

As we can see from this comment, we think auctioneerCut will be 0 when liquidatorCut = 0.

Btw when the admin sets auctioneerReward here, there are no requirements that auctioneerReward must be less than 1e18 and this one might be set to 1e18 by mistake.

So from this calculation, auctioneerCut would be whole amount and liquidatorCut = 0 when auctioneerReward = 1e18.

Then Witch._payInk() won't work at all because this condition is false.

This scenario would be possible when auctioneerReward is almost the same as 1e18(not exactly 1e18, like 1e18 * 0.9999) and liquidatorCut is small enough.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend modifying _payInk() like below.

function _payInk(
    DataTypes.Auction memory auction_,
    address to,
    uint256 liquidatorCut,
    uint256 auctioneerCut
) internal {
    if (liquidatorCut > 0 || auctioneerCut > 0) {
        IJoin ilkJoin = ladle.joins(auction_.ilkId);
        require(ilkJoin != IJoin(address(0)), "Join not found");

        // Pay auctioneer's cut if necessary
        if (auctioneerCut > 0) {
            ilkJoin.exit(auction_.auctioneer, auctioneerCut.u128());
        }

        // Give collateral to the liquidator
        if(liquidatorCut > 0) {
            ilkJoin.exit(to, liquidatorCut.u128());
        }
    }
}

alcueca commented 2 years ago

While true, this issue only happens when using a parameter configuration that can only be ascribed to a botched governance change, and as such I'd suggest reducing the risk to low.

PierrickGT commented 2 years ago

@alcueca before downgrading this issue to a QA Report one, I want to make sure this issue should not be marked as invalid instead. In the README you mention that parameters of governance functions are not checked because you have some on-chain testing scripts in place to avoid any governance error. Since this issue would only happen if the governance parameter auctioneerReward is not set properly, I'm inclined to label this issue as invalid.