Timeswap contest - sirhashalot's results

Handle

sirhashalot

Vulnerability details

Impact

The CollateralizedDebt.sol contract is a ERC721 token. It has a mint() function, which uses the underlying safeMint() function to create an ERC721 token representing a collateral position. The burn() function in CollateralizedDebt.sol should reverse the actions of mint() by burning the ERC721 token, but the ERC721 _burn() function is never called. This means a user can continue to hold their ERC721 token representing their position after receiving their funds. This is unlike the burn() function found in Bond.sol, Insurance.sol, and Liquidity.sol, which all call the _burn() function (though note the _burn() function in these other Timeswap Convenience contracts is the ERC20 _burn()).

Proof of Concept

The problematic burn() function is found in CollareralizedDebt.sol https://github.com/code-423n4/2022-01-timeswap/blob/bf50d2a8bb93a5571f35f96bd74af54d9c92a210/Timeswap/Timeswap-V1-Convenience/contracts/CollateralizedDebt.sol#L80-L88

Compare this function to the burn() functions defined in the other Timeswap Convenience contracts, which contain calls to _burn()

Recommended Mitigation Steps

Include the following line in the burn() function _burn(id);

Handle

sirhashalot

Vulnerability details

Impact

The safeDecimals() function, found in the SafeMetadata.sol contract and called in 3 different Timeswap Convenience contracts, can cause a revert. This is because the safeDecimals function attempts to use abi.decode to return a uint8 when data.length >= 32. However, a data.length value greater than 32 will cause abi.decode to revert.

A similar issue was found in a previoud code4rena contest: https://github.com/code-423n4/2021-05-nftx-findings/issues/46

Proof of Concept

The root cause is line 28 of the safeDecimals() function in SafeMetadata.sol

The following link shows the safeDecimals() function in the BoringCrypto library, which might be where this code was borrowed from, uses the strict equality check data.length == 32 https://github.com/boringcrypto/BoringSolidity/blob/ccb743d4c3363ca37491b87c6c9b24b1f5fa25dc/contracts/libraries/BoringERC20.sol#L54

safeDecimals() is used in multiple functions such as

• CollateralizedDebt.sol line 50 and line 54
• Bond.sol line 34
• Insurance.sol line 36

Recommended Mitigation Steps

Modify the safeDecimals() function to change >= 32 to == 32 like this if (success && data.length == 32) return abi.decode(data, (uint8));

Handle

sirhashalot

Vulnerability details

Impact

The safeName() function, found in the SafeMetadata.sol contract and called in 4 Timeswap Convenience contracts in the name() functions, can cause a revert. This could make the 4 contracts not compliant with the ERC20 standard for certain asset pairs, because the name() function should return a string and not revert.

The root cause of the issue is that the safeName() function assumes the return type of any ERC20 token to be a string. If the return value is not a string, abi.decode() will revert, and this will cause the name() functions in the Timeswap ERC20 contracts to revert. There are some tokens that aren't compliant, such as Sai from Maker, which returns a bytes32 value: https://kauri.io/#single/dai-token-guide-for-developers/#token-info

Because this is known to cause issues with tokens that don't fully follow the ERC20 spec, the safeName() function in the BoringCrypto library has a fix for this. The BoringCrypto safeName() function is similar to the one in Timeswap but it has a returnDataToString() function that handles the case of a bytes32 return value for a token name: https://github.com/boringcrypto/BoringSolidity/blob/ccb743d4c3363ca37491b87c6c9b24b1f5fa25dc/contracts/libraries/BoringERC20.sol#L15-L47

Proof of Concept

The root cause is line 12 of the safeName() function in SafeMetadata.sol

The safeName() function is called in:

• Bond.sol
• CollateralizedDebt.sol
• Insurance.sol
• Liquidity.sol

Recommended Mitigation Steps

Use the BoringCrypto safeName() function code to handle the case of a bytes32 return value: https://github.com/boringcrypto/BoringSolidity/blob/ccb743d4c3363ca37491b87c6c9b24b1f5fa25dc/contracts/libraries/BoringERC20.sol#L15-L47

Handle

sirhashalot

Vulnerability details

Impact

The safeSymbol() function, found in the SafeMetadata.sol contract and called in 4 Timeswap Convenience contracts in the symbol() functions, can cause a revert. This could make the 4 contracts not compliant with the ERC20 standard for certain asset pairs, because the symbol() function should return a string and not revert.

The root cause of the issue is that the safeSymbol() function assumes the return type of any ERC20 token to be a string. If the return value is not a string, abi.decode() will revert, and this will cause the symbol() functions in the Timeswap ERC20 contracts to revert.

Because this is known to cause issues with tokens that don't fully follow the ERC20 spec, the safeSymbol() function in the BoringCrypto library has a fix for this. The BoringCrypto safeSymbol() function is similar to the one in Timeswap but it has a returnDataToString() function that handles the case of a bytes32 return value for a token name: https://github.com/boringcrypto/BoringSolidity/blob/ccb743d4c3363ca37491b87c6c9b24b1f5fa25dc/contracts/libraries/BoringERC20.sol#L15-L39

Proof of Concept

The root cause is line 20 of the safeSymbol() function in SafeMetadata.sol

The safeSymbol() function is called in:

• Bond.sol
• CollateralizedDebt.sol
• Insurance.sol
• Liquidity.sol

Recommended Mitigation Steps

Use the BoringCrypto safeSymbol() function code with the returnDataToString() parsing function to handle the case of a bytes32 return value: https://github.com/boringcrypto/BoringSolidity/blob/ccb743d4c3363ca37491b87c6c9b24b1f5fa25dc/contracts/libraries/BoringERC20.sol#L15-L39