SIZE contest - 8olidity's results

Lines of code

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L391-L410

Vulnerability details

Impact

In cancelAuction, there are statements to determine whether to call before finalize(). Because createAuction() assigns the value of a.dat.lowestquote to type(uint128).max.

    function cancelAuction(uint256 auctionId) external {
        Auction storage a = idToAuction[auctionId];
        if (msg.sender != a.data.seller) {
            revert UnauthorizedCaller();
        }
        // Only allow cancellations before finalization
        // Equivalent to atState(idToAuction[auctionId], ~STATE_FINALIZED)
        if (a.data.lowestQuote != type(uint128).max) {
            revert InvalidState();
        }

        // Allowing bidders to cancel bids (withdraw quote)
        // Auction considered forever States.AcceptingBids but nobody can finalize
        a.data.seller = address(0);
        a.timings.endTimestamp = type(uint32).max;

        emit AuctionCancelled(auctionId);

        SafeTransferLib.safeTransfer(ERC20(a.params.baseToken), msg.sender, a.params.totalBaseAmount);
    }

However, he just rely on a.data.lowstquote! = type(uint128).max to judge, but if in finalize(), the clearingquote that was passed in was type(uint128).max. Then you can bypass his judgment. And there is no restriction on Clearingquote in finalize().

Proof of Concept

clearingQuote = type(uint128).max

Tools Used

vscode

Recommended Mitigation Steps

Judging clearingquote in finalize()

Lines of code

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L163

Vulnerability details

Impact

solmate won't check if the token is a contract or not.

A hacker could set traps for non existing tokens to steal future funds from users.

ref: https://github.com/code-423n4/2022-08-olympus-findings/issues/489

Proof of Concept

The safeTransfer() functions used in the contract are wrappers around the solmate library. Solmate will not check for contract existance.

File: src/libraries/TransferHelper.sol
function safeTransferFrom(
    ERC20 token,
    address from,
    address to,
    uint256 amount
) internal {
    (bool success, bytes memory data) = address(token).call(
        abi.encodeWithSelector(ERC20.transferFrom.selector, from, to, amount)
    );

    require(success && (data.length == 0 || abi.decode(data, (bool))), "TRANSFER_FROM_FAILED");
}

function safeTransfer(
    ERC20 token,
    address to,
    uint256 amount
) internal {
    (bool success, bytes memory data) = address(token).call(
        abi.encodeWithSelector(ERC20.transfer.selector, to, amount)
    );

    require(success && (data.length == 0 || abi.decode(data, (bool))), "TRANSFER_FAILED");
}

Tools Used

vscode

Recommended Mitigation Steps

Looking at OpenZeppelin's implementation, a check is made on the code for the token address.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L36-L42

address(token).code.length > 0

Consider using OpenZeppelin or checking for contract existance manually inside TransferHelper.sol.