Consider also introducing a _MAX_DELAY in Preparable contract to avoid situations when deadline cannot be bet due to unreasonable delay.
There are TODOs left in the code. While this does not cause any direct issue, it indicates a bad smell and uncertainty and makes it harder for an auditor to make assumptions:
// TODO: add constant gas consumed for transfer and tx prologue
// TODO Add validation of curve pools
// TODO Test validation
Better avoid explicit casting and use a SafeCast library, e.g.:
While this may be an intended behavior, I think Uniswap is more widely used and supports more pairs, so it might be a good idea to default to it instead.
You may also consider a more universal approach, e.g. token to dex mapping and general adapter for each dex to support not just these 2 dexes, but have more flexibility.
function register in TopUpAction could have a max totalLockAmount parameter to let the users control the slippage especially when depositToken should be swapped to actionToken.
Tokens having more than 18 decimals are not supported, the calculation will revert here:
In contract TopUpAction ScaledMath is used with uint128:
using ScaledMath for uint128;
ScaledMath is written to support only uint256 type, so you shouldn't use it with lower types as they are automatically converted to uint256 and thus overflow/underflow might be escaped.
Consider also introducing a _MAX_DELAY in Preparable contract to avoid situations when deadline cannot be bet due to unreasonable delay.
There are TODOs left in the code. While this does not cause any direct issue, it indicates a bad smell and uncertainty and makes it harder for an auditor to make assumptions:
Better avoid explicit casting and use a SafeCast library, e.g.:
SUSHISWAP is a default dex. If swapViaUniswap is not explicitly set to true for a token, then _getDex will default to _SUSHISWAP:
While this may be an intended behavior, I think Uniswap is more widely used and supports more pairs, so it might be a good idea to default to it instead.
You may also consider a more universal approach, e.g. token to dex mapping and general adapter for each dex to support not just these 2 dexes, but have more flexibility.
function register in TopUpAction could have a max totalLockAmount parameter to let the users control the slippage especially when depositToken should be swapped to actionToken.
Tokens having more than 18 decimals are not supported, the calculation will revert here:
.transfer is used for transfering ether, e.g.:
It is currently not recommended as recipients with custom fallback functions (smart contracts) will not be able to handle that. You can read more here: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/
Solution (don't forget re-entrancy protection): https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L53-L59
ScaledMath is written to support only uint256 type, so you shouldn't use it with lower types as they are automatically converted to uint256 and thus overflow/underflow might be escaped.