Audit fix 94 one approve #17
Conversation
contracts/SushiYieldSource.sol
Outdated
| ///@notice Approve SUSHI to spend infinite sushiBar (xSUSHI) | ||
| /// @dev No initializer flag required | ||
| function intialize() external { | ||
| sushiAddr.safeApprove(address(sushiBar), type(uint256).max); |
There was a problem hiding this comment.
Is this safe or do we need the intiailizer modifier?
There was a problem hiding this comment.
@asselstine Is it safe to have public access to this function?
There was a problem hiding this comment.
Yes, because the SushiBar is an immutable contract that is very simple
contracts/SushiYieldSource.sol
Outdated
|
|
||
| ///@notice Approve SUSHI to spend infinite sushiBar (xSUSHI) | ||
| /// @dev No initializer flag required | ||
| function intialize() external { |
There was a problem hiding this comment.
This should really be called 'approveMax' or something
There was a problem hiding this comment.
I've added the function here: 40953c8
We need to use safeIncreaseAllowance, otherwise approve will always revert with SafeERC20: approve from non-zero to non-zero allowance cause we've already approved the max amount in the constructor.
There was a problem hiding this comment.
Also, should we add the sponsor and transferERC20 functions?
This way we gonna have to add the onlyOwnerOrAssetManager modifier and we can then use the isOwner function for approveMaxAmount.
contracts/SushiYieldSource.sol
Outdated
|
|
||
| mapping(address => uint256) public balances; | ||
|
|
||
| constructor(ISushiBar _sushiBar, ISushi _sushiAddr) public { | ||
| constructor(ISushiBar _sushiBar, IERC20 _sushiAddr) public { | ||
| sushiBar = _sushiBar; | ||
| sushiAddr = _sushiAddr; |
There was a problem hiding this comment.
You should approve max here in the constructor as well, so that the yield source is ready
PierrickGT
force-pushed
the
audit-fix-94-one-approve
branch
2 times, most recently
from
4030e79
to
40953c8
Compare
contracts/SushiYieldSource.sol
Outdated
| /// @return The actual amount of tokens that were redeemed. This may be different from the amount passed due to the fractional math involved. | ||
| function redeemToken(uint256 amount) public override returns (uint256) { | ||
| /// @return The actual amount of tokens that were redeemed. This may be different from the amount passed due to the fractional math involved. | ||
| function redeemToken(uint256 amount) external override returns (uint256) { |
There was a problem hiding this comment.
could use a reentrancy guard....is this covered elsewhere?
PierrickGT
force-pushed
the
audit-fix-94-one-approve
branch
from
40953c8
to
0e6c145
Compare
PierrickGT
force-pushed
the
audit-fix-94-one-approve
branch
from
0e6c145
to
abf108f
Compare
PierrickGT
force-pushed
the
audit-fix-94-one-approve
branch
from
abf108f
to
913afe2
Compare
as per code-423n4/2021-06-pooltogether-findings#94