Tempus Finance contest - TomFrenchBlockchain's results

Handle

TomFrench

Vulnerability details

Impact

Increased gas costs on several TempusPool functions

Proof of Concept

TempusPools have a finalize function which checks whether block.timestamp >= maturityTime and flips the maturity storage variable as well as setting maturityInterestRate to the current interest rate.

https://github.com/tempus-finance/tempus-protocol/blob/0240b4d172d7aa093a70e0401f4140c99aa30dc6/contracts/TempusPool.sol#L126-L135

maturity is used in several places to check whether the pool has expired however checking this variable is more expensive than checking block.timestamp >= maturityTime (due to the need for a SLOAD whereas maturityTime is immutable so no SLOAD is needed). I'd recommend making a function matured() public view to keep the readability.

However maturityInterestRate still needs to be set correctly. This could be done by reading the current interest rate when matured() returns true but maturityInterestRate == 0 this could cause issues with some functions which are currently view functions however.

Recommended Mitigation Steps

Half solution:

Replace matured state variable with a matured() view function which returns maturityInterestRate > 0. This removes an SSTORE from finalize and an SLOAD from any function which uses maturityInterestRate as you can just check if it's greater than zero to see if the pool has matured.

Full solution:

Replace matured state variable with a matured() view function which returns block.timestamp >= maturityTime. This combined with setting maturityInterestRate when you see that matured() == true and maturityInterestRate == 0 would remove the need for the finalize function entirely.

Handle

TomFrench

Vulnerability details

Impact

Reduced flexibility of AMM + additional gas costs on swaps

Proof of Concept

As the relative payouts of the principal and yield tokens are fixed at the point of finalisation, there's no need to freeze the AMM as it will just rapidly be arbed to the final prices of each token. No funds will be lost by LPs.

Making this change would reduce gas costs as swaps won't have to check maturity (load the TempusPool then perform SLOAD for maturity state variable).

Recommended Mitigation Steps

Remove beforeMaturity modifier from AMM.

Handle

TomFrench

Vulnerability details

Impact

Increased gas costs

Proof of Concept

On L189, we use a uint8 as the for loop variable: https://github.com/code-423n4/2021-10-tempus/blob/63f7639aad08f2bba717830ed81e0649f7fc23ee/contracts/amm/TempusAMM.sol#L189

Due to how the EVM natively works on 256 numbers, using a 8 bit number here introduces additional costs as the EVM has to properly enforce the limits of this smaller type.

See the warning at this link: https://docs.soliditylang.org/en/v0.8.0/internals/layout_in_storage.html#layout-of-state-variables-in-storage

Recommended Mitigation Steps

Change i to be a uint256 and replace any similar uints which only exist in memory and aren't required to use a smaller type.

Handle

TomFrench

Vulnerability details

Impact

Proof of Concept

As the TempusAmm only ever registers with the Balancer Vault with the two token specialization the GeneralPool interface will never be used as the Vault will call the MinimalSwapInfoPool hooks instead.

https://github.com/code-423n4/2021-10-tempus/blob/63f7639aad08f2bba717830ed81e0649f7fc23ee/contracts/amm/TempusAMM.sol#L107-L110

See here in the Balancer Vault code:

https://github.com/balancer-labs/balancer-v2-monorepo/blob/62c5cba7cae1d481f913c90fe0d9d94e101570c5/pkg/vault/contracts/Swaps.sol#L287-L292

Recommended Mitigation Steps

Remove the inheritance from BaseGeneralPool and remove the functions highlighted in the link below. This will help reduce bytecode from the AMM factory and reduce deployment costs.

https://github.com/code-423n4/2021-10-tempus/blob/63f7639aad08f2bba717830ed81e0649f7fc23ee/contracts/amm/TempusAMM.sol#L283-L309

Handle

TomFrench

Vulnerability details

Impact

Higher gas costs on transfers of tokens from user to TempusPool

Proof of Concept

Following the flow of tokens from the user to their the TempusPool contract:

1. User calls TempusController.depositBacking, TempusController transfers user's tokens to itself and approves relevant TempusPool

https://github.com/code-423n4/2021-10-tempus/blob/63f7639aad08f2bba717830ed81e0649f7fc23ee/contracts/TempusController.sol#L412-L414

2. TempusController calls TempusPool.deposit which in turn transfers tokens from the TempusController and then invests them.

https://github.com/code-423n4/2021-10-tempus/blob/63f7639aad08f2bba717830ed81e0649f7fc23ee/contracts/TempusPool.sol#L178

This first transfer is then superfluous as the TempusPool trusts the TempusController (it's the only contract which may call deposit). We're then incurring the costs of 1 transfer and 1 approve unnecessarily.

Recommended Mitigation Steps

As TempusPool trusts TempusController, TempusController can transfer the tokens directly to TempusPool and just tell it how much has been deposited.

L412-L414 of TempusController.sol would then be replaced with:

// Deposit to directly to targetPool
uint transferredYBT = yieldBearingToken.untrustedTransferFrom(msg.sender, targetPool, yieldTokenAmount);