Possible casting overflow in _updateAccounting function

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L409-L414 https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L344-L361 https://github.com/code-423n4/2022-07-yield/blob/6ab092b8c10e4dabb470918ae15c6451c861655f/contracts/Witch.sol#L443-L444

Vulnerability details

Impact

In the _updateAccounting function, the inkOut and artIn parameters are cast from type uint256 to uint128. However, since the explicit cast does not ensure the value fits the uint128 data type (i.e., not ensuring value <= type(uint128).max), casting overflows are possible, causing the results to be incorrect.

Proof of Concept

The parameters are accepted as uint128 in the below code;

    function _updateAccounting(
        bytes12 vaultId,
        DataTypes.Auction memory auction_,
        uint256 inkOut,
        uint256 artIn
    ) internal {

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

But the previous function -payFYToken()- which calls _updateAccounting function internally has the parameters in different datatype - uint256;

    function payFYToken(
        bytes12 vaultId,
        address to,
        uint128 minInkOut,
        uint128 maxArtIn
    )
        external
        returns (
            uint256 liquidatorCut,
            uint256 auctioneerCut,
            uint256 artIn
        )
    {
        DataTypes.Auction memory auction_ = auctions[vaultId];
        require(auction_.start > 0, "Vault not under auction");

        // If offering too much fyToken, take only the necessary.
        artIn = maxArtIn > auction_.art ? auction_.art : maxArtIn;

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

And finally, both parameters are cast to uint128 as below;

                auction_.ink -= inkOut.u128();
                auction_.art -= artIn.u128();

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

An actor who can manage to artificially create max debt in uint256 type in a vault can pay his debt with a lower value in an auction that he initiated due to this overflow.

Tools Used

Manual review

Recommended Mitigation Steps

The team might consider adding casting overflow checks for the above-mentioned lines.

[KenzoAgada]

The casting is done using u128() which is a yield-utils function which checks for overflow, see CastU256U128 :

vlibrary CastU256U128 {
    /// @dev Safely cast an uint256 to an uint128
    function u128(uint256 x) internal pure returns (uint128 y) {
        require (x <= type(uint128).max, "Cast overflow");
        y = uint128(x);

[0xSorryNotSorry]

u128() function is not used when passing parameters from payFYToken() internally to _updateAccounting function at the input level. @KenzoAgada

[KenzoAgada]

Hmm... _updateAccounting receives inkOut and artIn as uint256. But in the end it converts them to uint128, reverting if they don't fit. So it will revert. Am I missing something?

[0xSorryNotSorry]

You're right @KenzoAgada. I was confused assuming the flow on the other direction uint256 -> uint128

[alcueca]

Disputed, @KenzoAgada explained why.

[PierrickGT]

Agree with Kenzo, the casting to uint128 happens in the _updateAccounting function. So it will revert there if the value doesn't fit in a uint128. For this reason, I have labelled this issue as invalid.