code423n4 commented 3 years ago

Handle

JMukesh

Vulnerability details

Impact

Different definition of beforeMaturity() and afterMaturity() modifier in strategy.sol and Fytoken.sol which used FYTokenFacory()

Fytoken.sol

In modifier beforeMaturity(), timestamp should be strictly less than maturity

require( uint32(block.timestamp) < maturity, "Only before maturity" );

where as in strategy.sol

modifier beforeMaturity() , timestamp should <= maturity due to which we can call mint() even if timestamp == maturtiy

require ( fyToken.maturity() >= uint32(block.timestamp), "Only before maturity" );

same is used for afterMaturity() modifier but in this case

strategy.sol have strict condition that, timestamp < maturity

require ( fyToken == IFYToken(address(0)) || fyToken.maturity() < uint32(block.timestamp), "Only after maturity" );

and FyToken.sol have

require( uint32(block.timestamp) >= maturity, "Only after maturity" );

here mature(), accrual(), redeem() can be called if timestamp == maturity

Proof of Concept

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/yieldspace/Strategy.sol#L82

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/FYToken.sol#L65

Tools Used

manual review

Recommended Mitigation Steps

same definition of modifier should be used otherwise it increase the chances of error

alcueca commented 3 years ago

It is right that there is a different definition of before and after maturity, and that Strategy.sol should match FYToken.sol, same as Pool.sol does.

However, there is no impact from this issue. The only thing that could happen is that an user mint strategy tokens on an already matured pool, which is harmless.

alcueca commented 3 years ago

Fix

ghoul-sol commented 3 years ago

per sponsor comment, making this low risk

code-423n4 / 2021-08-yield-findings