Platform: Code4rena
Start Date: 14/11/2021
Pot Size: $30,000 USDC
Total HM: 7
Participants: 13
Period: 3 days
Judge: leastwood
Total Solo HM: 4
Id: 57
League: ETH
Rank: 1/13
Findings: 7
Award: $12,462.74
🌟 Selected for report: 14
🚀 Solo Findings: 2
🌟 Selected for report: WatchPug
Also found by: 0x0x0x, Meta0xNull, fatima_naz, gzeon, ksk2345
WatchPug
function setGuardian(address _guardian) external { _onlyGovernance(); governance = _guardian; }
function setGuardian(address _guardian) external { _onlyGovernance(); governance = _guardian; }
governance = _guardian should be guardian = _guardian.
WatchPug
function redeem(IERC20 token, uint amount, uint poolId, int128 idx, uint minOut) external defend blockLocked whenNotPaused returns(uint out) { ibbtc.safeTransferFrom(msg.sender, address(this), amount); Pool memory pool = pools[poolId]; if (poolId < 3) { // setts settPeak.redeem(poolId, amount); pool.sett.withdrawAll(); pool.deposit.remove_liquidity_one_coin(pool.lpToken.balanceOf(address(this)), idx, minOut); } else if (poolId == 3) { // byvwbtc byvWbtcPeak.redeem(amount); IbyvWbtc(address(pool.sett)).withdraw(); // withdraws all available } else { revert("INVALID_POOL_ID"); } out = token.balanceOf(address(this)); token.safeTransfer(msg.sender, out); }
In the current implementation of. Zap.sol#redeem(), the outAmount of IbyvWbtc.withdraw() is not controlled by minOut.
Consider implementing the minOut check in between L236 and L237.
... out = token.balanceOf(address(this)); require(out >= _minOut, "Slippage Check"); token.safeTransfer(msg.sender, out); }
#0 - GalloDaSballo
2021-11-17T15:05:40Z
Agree with the finding, not having slippage check at end means people can get rekt, we'll add as suggested
🌟 Selected for report: WatchPug
2821.3259 USDC - $2,821.33
WatchPug
function setZapConfig( uint256 _idx, address _sett, address _token, address _curvePool, address _withdrawToken, int128 _withdrawTokenIndex ) external { _onlyGovernance(); require(_sett != address(0)); require(_token != address(0)); require( _withdrawToken == address(WBTC) || _withdrawToken == address(RENBTC) ); zapConfigs[_idx].sett = ISett(_sett); zapConfigs[_idx].token = IERC20Upgradeable(_token); zapConfigs[_idx].curvePool = ICurveFi(_curvePool); zapConfigs[_idx].withdrawToken = IERC20Upgradeable(_withdrawToken); zapConfigs[_idx].withdrawTokenIndex = _withdrawTokenIndex; }
In the current implementation, when curvePool or token got updated, token is not approved to curvePool, which will malfunction the contract and break minting.
Change to:
function setZapConfig( uint256 _idx, address _sett, address _token, address _curvePool, address _withdrawToken, int128 _withdrawTokenIndex ) external { _onlyGovernance(); require(_sett != address(0)); require(_token != address(0)); require( _withdrawToken == address(WBTC) || _withdrawToken == address(RENBTC) ); if (zapConfigs[_idx].curvePool != _curvePool && _curvePool != address(0)) { IERC20Upgradeable(_token).safeApprove( _curvePool, type(uint256).max ); } zapConfigs[_idx].sett = ISett(_sett); zapConfigs[_idx].token = IERC20Upgradeable(_token); zapConfigs[_idx].curvePool = ICurveFi(_curvePool); zapConfigs[_idx].withdrawToken = IERC20Upgradeable(_withdrawToken); zapConfigs[_idx].withdrawTokenIndex = _withdrawTokenIndex; }
#0 - GalloDaSballo
2021-11-17T14:40:40Z
Agree with the finding, it should be noted that adding a pool does handle for the scenario, this would break the pool in case we update it or change the token
#1 - GalloDaSballo
2021-11-17T14:41:58Z
Arguably for severity:
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
As this would break only if we update the pool
W will mitigate by following the warden advice
#2 - 0xleastwood
2021-12-09T12:53:33Z
agree with sponsor, will mark this as medium as assets are not at direct risk ^
🌟 Selected for report: WatchPug
2821.3259 USDC - $2,821.33
WatchPug
The check for RENCRV_VAULT.blockLock is only needed when if (_amounts[1] > 0 || _amounts[2] > 0).
However, in the current implementation, the check is done at the very first, making transactions unrelated to RENCRV_VAULT fail unexpectedly if there is a prior transaction involved with RENCRV_VAULT in the same block.
function deposit(uint256[4] calldata _amounts, uint256 _minOut) public whenNotPaused { // Not block locked by setts require( RENCRV_VAULT.blockLock(address(this)) < block.number, "blockLocked" ); require( IBBTC_VAULT.blockLock(address(this)) < block.number, "blockLocked" ); uint256[4] memory depositAmounts; for (uint256 i = 0; i < 4; i++) { if (_amounts[i] > 0) { ASSETS[i].safeTransferFrom( msg.sender, address(this), _amounts[i] ); if (i == 0 || i == 3) { // ibbtc and sbtc depositAmounts[i] += _amounts[i]; } } } if (_amounts[1] > 0 || _amounts[2] > 0) { // Use renbtc and wbtc to mint ibbtc // NOTE: Can change to external zap if implemented depositAmounts[0] += _renZapToIbbtc([_amounts[1], _amounts[2]]); } // ... }
#0 - GalloDaSballo
2021-11-17T15:08:06Z
Agree with the finding, we would have to check for those locks only under specific condition, not doing so opens up to unnecessary reverts
#1 - GalloDaSballo
2021-11-17T15:08:15Z
We have mitigated by following the advice of the warden
WatchPug
function deposit(uint256[4] calldata _amounts) public whenNotPaused { // ... }
Given that IbbtcVaultZap.sol#deposit() will add liquidity to the curve pool, and the amount out differs when the price of tokens in the pool changes.
However, the current implementation provides no parameter for slippage control, making them vulnerable to front-run attacks. Especially for transactions with rather large volumes.
Consider adding a minAmountOut parameter.
#0 - GalloDaSballo
2021-11-17T15:07:02Z
Agree with the finding, we have mitigated by adding a slippage check minOut
#1 - 0xleastwood
2021-12-09T13:08:40Z
duplicate of #71
🌟 Selected for report: WatchPug
WatchPug
token should be validated to make sure the user can get the redeemed tokens.
function redeem(IERC20 token, uint amount, uint poolId, int128 idx, uint minOut) external defend blockLocked whenNotPaused returns(uint out) { ibbtc.safeTransferFrom(msg.sender, address(this), amount); Pool memory pool = pools[poolId]; if (poolId < 3) { // setts settPeak.redeem(poolId, amount); pool.sett.withdrawAll(); pool.deposit.remove_liquidity_one_coin(pool.lpToken.balanceOf(address(this)), idx, minOut); } else if (poolId == 3) { // byvwbtc byvWbtcPeak.redeem(amount); IbyvWbtc(address(pool.sett)).withdraw(); // withdraws all available } else { revert("INVALID_POOL_ID"); } out = token.balanceOf(address(this)); token.safeTransfer(msg.sender, out); }
#0 - GalloDaSballo
2021-11-17T16:38:07Z
I agree with the finding, although other wardens proved that the wrong token can cause loss of funds, we will add a slippage check at the end of the function
🌟 Selected for report: WatchPug
WatchPug
if (_amounts[1] > 0 || _amounts[2] > 0) { // Use renbtc and wbtc to mint ibbtc // NOTE: Can change to external zap if implemented depositAmounts[0] += _renZapToIbbtc([_amounts[1], _amounts[2]]); }
Even though it's unlikely to overflow in this particular case, we still recommend using SafeMath instead.
#0 - GalloDaSballo
2021-11-17T16:30:06Z
Ambivalent on the finding esp because bitcoin tokens tend to have 8 decimals
#1 - 0xleastwood
2021-12-09T12:49:50Z
agree with warden, this is best practice so keeping issue open as low.
🌟 Selected for report: WatchPug
940.442 USDC - $940.44
WatchPug
require( RENCRV_SETT.blockLock(address(IBBTC_MINT_ZAP)) < block.number, "blockLocked" );
Considering that the RENCRV_SETT contract is also controlled by BadgerDAO and it's upgradable.
We suggest upgrading it and whitelisting the zap contracts.
#0 - tabshaikh
2021-11-16T15:37:22Z
Gonna do that
#1 - GalloDaSballo
2021-11-17T15:09:19Z
Disagree with the finding, the blocklock is a feature not a bug, if this were to be a repeated issue we would have to change a lot more
#2 - 0xleastwood
2021-12-09T13:14:39Z
agree with sponsor, marking as low risk as it is still a useful to consider implementing such a change.
🌟 Selected for report: WatchPug
WatchPug
Given the fact that the zap contracts will be layered, one mint() function may call another mint() function and tokens will be transferred multiple times from contract to contract. If we can add a recipient parameter, the unnecessary contract to contract token transfers can be avoided and save gas.
For example:
SettToRenIbbtcZap.sol#mint() will call IBBTC_MINT_ZAP.mint(), and IBBTC_MINT_ZAP will mint and transfer the minted tokens back to SettToRenIbbtcZap contract, and it will then send to the user (msg.sender)
// Use other zap to deposit uint256 ibbtcAmount = IBBTC_MINT_ZAP.mint( address(zapConfig.withdrawToken), btcAmount, 0, // poolId - renCrv: 0 address(zapConfig.withdrawToken) == address(RENBTC) ? 0 : 1, // idx - renbtc: 0, wbtc: 1 _minOut ); IBBTC.safeTransfer(msg.sender, ibbtcAmount); return ibbtcAmount; }
If a recipient parameter get added to IBBTC_MINT_ZAP.mint(), the above codes can be changed to:
// Use other zap to deposit return IBBTC_MINT_ZAP.mint( address(zapConfig.withdrawToken), btcAmount, 0, // poolId - renCrv: 0 address(zapConfig.withdrawToken) == address(RENBTC) ? 0 : 1, // idx - renbtc: 0, wbtc: 1 _minOut, msg.sender // recipient ); }
#0 - GalloDaSballo
2021-11-17T16:42:44Z
I appreciate the advice, i think there would be some unintended consequences, but will think about it, thanks!
🌟 Selected for report: WatchPug
WatchPug
Check if poolId <= 3 earlier can avoid unnecessary code execution when this check failed.
function mint(IERC20 token, uint amount, uint poolId, uint idx, uint minOut) external defend blockLocked whenNotPaused returns(uint _ibbtc) { token.safeTransferFrom(msg.sender, address(this), amount); Pool memory pool = pools[poolId]; if (poolId < 3) { // setts _addLiquidity(pool.deposit, amount, poolId + 2, idx); // pools are such that the #tokens they support is +2 from their poolId. pool.sett.deposit(pool.lpToken.balanceOf(address(this))); _ibbtc = settPeak.mint(poolId, pool.sett.balanceOf(address(this)), new bytes32[](0)); } else if (poolId == 3) { // byvwbtc IbyvWbtc(address(pool.sett)).deposit(new bytes32[](0)); // pulls all available _ibbtc = byvWbtcPeak.mint(pool.sett.balanceOf(address(this)), new bytes32[](0)); } else { revert("INVALID_POOL_ID"); }
Change to:
function mint(IERC20 token, uint amount, uint poolId, uint idx, uint minOut) external defend blockLocked whenNotPaused returns(uint _ibbtc) { require(poolId <= 3, "INVALID_POOL_ID"); token.safeTransferFrom(msg.sender, address(this), amount); Pool memory pool = pools[poolId]; if (poolId < 3) { // setts _addLiquidity(pool.deposit, amount, poolId + 2, idx); // pools are such that the #tokens they support is +2 from their poolId. pool.sett.deposit(pool.lpToken.balanceOf(address(this))); _ibbtc = settPeak.mint(poolId, pool.sett.balanceOf(address(this)), new bytes32[](0)); } else { // byvwbtc IbyvWbtc(address(pool.sett)).deposit(new bytes32[](0)); // pulls all available _ibbtc = byvWbtcPeak.mint(pool.sett.balanceOf(address(this)), new bytes32[](0)); }
#0 - GalloDaSballo
2021-11-17T16:42:11Z
Agree with the finding
🌟 Selected for report: WatchPug
WatchPug
Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.
for (uint i = 0; i < pools.length; i++) { Pool memory pool = pools[i]; pool.lpToken.safeApprove(address(pool.sett), uint(-1)); if (i < 3) { ren.safeApprove(address(pool.deposit), uint(-1)); wbtc.safeApprove(address(pool.deposit), uint(-1)); IERC20(address(pool.sett)).safeApprove(address(settPeak), uint(-1)); } else { IERC20(address(pool.sett)).safeApprove(address(byvWbtcPeak), uint(-1)); } }
As pools.length must be 4, it can be replaced with a literal 4.
#0 - GalloDaSballo
2021-11-17T16:38:43Z
Agree with the finding as the warden actually explained the logic
🌟 Selected for report: WatchPug
Also found by: GiveMeTestEther, pmerkleplant, ye0lde
14.6747 USDC - $14.67
WatchPug
Unused local variables in contracts increase contract size and gas usage at deployment.
Instances include:
function calcMintWithRen(uint amount) public view returns(uint poolId, uint idx, uint bBTC, uint fee) { uint _ibbtc; uint _fee; ...
function calcMintWithWbtc(uint amount) public view returns(uint poolId, uint idx, uint bBTC, uint fee) { uint _ibbtc; uint _fee; ...
uint _fee; uint _ren;
uint _fee; uint _wbtc;
#0 - tabshaikh
2021-11-16T15:53:53Z
fixed, duplicate of #6
🌟 Selected for report: WatchPug
WatchPug
function ibbtcToCurveLP(uint poolId, uint bBtc) public view returns(uint lp, uint fee) { uint sett; uint max; (sett,fee,max) = settPeak.calcRedeem(poolId, bBtc); Pool memory pool = pools[poolId]; if (bBtc > max) { return (0,fee); } else { // pesimistically charge 0.5% on the withdrawal. // Actual fee might be lesser if the vault keeps keeps a buffer uint strategyFee = sett.mul(controller.strategies(pool.lpToken).withdrawalFee()).div(10000); lp = sett.sub(strategyFee).mul(pool.sett.getPricePerFullShare()).div(1e18); fee = fee.add(strategyFee); } }
L309-311 is necessary as they won't affect the storage or the returns anyway.
Change to:
if (bBtc <= max) { // pesimistically charge 0.5% on the withdrawal. // Actual fee might be lesser if the vault keeps keeps a buffer uint strategyFee = sett.mul(controller.strategies(pool.lpToken).withdrawalFee()).div(10000); lp = sett.sub(strategyFee).mul(pool.sett.getPricePerFullShare()).div(1e18); fee = fee.add(strategyFee); }
#0 - GalloDaSballo
2021-11-17T16:33:32Z
Personally I like the return early, also like the idea presented by the warden, ambivalent on applying the improvement
🌟 Selected for report: WatchPug
WatchPug
uint256[4] memory depositAmounts; for (uint256 i = 0; i < 4; i++) { if (_amounts[i] > 0) { ASSETS[i].safeTransferFrom( msg.sender, address(this), _amounts[i] ); if (i == 0 || i == 3) { // ibbtc and sbtc depositAmounts[i] += _amounts[i]; } } }
depositAmounts[i] += _amounts[i] can be changed to depositAmounts[i] = _amounts[i] as depositAmounts[i] == 0.
#0 - GalloDaSballo
2021-11-17T16:27:53Z
Agree with the finding
🌟 Selected for report: WatchPug
WatchPug
settPeak.mint() and byvWbtcPeak.mint() are blockLocked, check if locked before calling them can allow blocklocked transactions to fail earlier and save gas.
function mint(IERC20 token, uint amount, uint poolId, uint idx, uint minOut) external defend blockLocked whenNotPaused returns(uint _ibbtc) { token.safeTransferFrom(msg.sender, address(this), amount); Pool memory pool = pools[poolId]; if (poolId < 3) { // setts _addLiquidity(pool.deposit, amount, poolId + 2, idx); // pools are such that the #tokens they support is +2 from their poolId. pool.sett.deposit(pool.lpToken.balanceOf(address(this))); _ibbtc = settPeak.mint(poolId, pool.sett.balanceOf(address(this)), new bytes32[](0)); } else if (poolId == 3) { // byvwbtc IbyvWbtc(address(pool.sett)).deposit(new bytes32[](0)); // pulls all available _ibbtc = byvWbtcPeak.mint(pool.sett.balanceOf(address(this)), new bytes32[](0)); } else { revert("INVALID_POOL_ID"); } require(_ibbtc >= minOut, "INSUFFICIENT_IBBTC"); // used for capping slippage in curve pools ibbtc.safeTransfer(msg.sender, _ibbtc); }
#0 - GalloDaSballo
2021-11-17T16:27:14Z
Agree with the finding
36.2338 USDC - $36.23
WatchPug
if (numTokens == 2) { uint[2] memory amounts; amounts[idx] = amount; pool.add_liquidity(amounts, 0); } if (numTokens == 3) { uint[3] memory amounts; amounts[idx] = amount; pool.add_liquidity(amounts, 0); } if (numTokens == 4) { uint[4] memory amounts; amounts[idx] = amount; pool.add_liquidity(amounts, 0); }
Change to else if can save gas:
if (numTokens == 2) { uint[2] memory amounts; amounts[idx] = amount; pool.add_liquidity(amounts, 0); } else if (numTokens == 3) { uint[3] memory amounts; amounts[idx] = amount; pool.add_liquidity(amounts, 0); } else if (numTokens == 4) { uint[4] memory amounts; amounts[idx] = amount; pool.add_liquidity(amounts, 0); }
#0 - GalloDaSballo
2021-11-17T16:34:04Z
I would like an explanation
You're stil comparing a condition and you're still making a jump, why is else if cheaper?
#1 - 0xleastwood
2021-12-09T11:44:09Z
duplicate of #76 which contains explanation.