Platform: Code4rena
Start Date: 02/08/2023
Pot Size: $42,000 USDC
Total HM: 13
Participants: 45
Period: 5 days
Judge: hickuphh3
Total Solo HM: 5
Id: 271
League: ETH
Rank: 11/45
Findings: 2
Award: $743.02
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: 0xmystery
Also found by: 0xbepresent, MohammedRizwan, Rolezn, bin2chen, rvierdiiev
284.0095 USDC - $284.01
| Issue | Contexts | |
|---|---|---|
| LOW‑1 | Function can risk gas exhaustion on large receipt calls due to multiple mandatory loops | 1 |
| LOW‑2 | Functions should be marked as payable when interacting with ETH like for transfering ETH to other contracts | 1 |
| LOW‑3 | Large transfers may not work with some ERC20 tokens | 5 |
| LOW‑4 | Unsafe downcast | 10 |
| LOW‑5 | Using zero as a parameter | 8 |
Total: 25 contexts over 5 issues
| Issue | Contexts | |
|---|---|---|
| NC‑1 | Condition will not revert when block.timestamp is == to the compared variable | 2 |
| NC‑2 | Functions that alter state should emit events | 2 |
| NC‑3 | Generate perfect code headers for better solidity code layout and readability | 2 |
| NC‑4 | Initial value check is missing in Set Functions | 2 |
| NC‑5 | Redundant Cast | 1 |
| NC‑6 | Remove forge-std import | 1 |
| NC‑7 | Remove unused error definition | 1 |
| NC‑8 | Use named function calls | 1 |
| NC‑9 | Zero as a function argument should have a descriptive meaning | 1 |
Total: 13 contexts over 9 issues
The function contains loops over arrays that the user cannot influence. In any call for the function, users spend gas to iterate over the array. There are loops in the following functions that iterate all array values. Which could risk gas exhaustion on large array calls.
This risk is small, but could be lessened somewhat by allowing separate calls for small loops. Consider allowing partial calls via array arguments, or detecting expensive call bundles and only partially iterating over them. Since the logic already filters out calls, this would make the later loops smaller.
File: RngRelayAuction.sol 131: function rngComplete( ... for (uint8 i = 0; i < _rewards.length; i++) { uint104 _reward = uint104(_rewards[i]); if (_reward > 0) { prizePool.withdrawReserve(auctionResults[i].recipient, _reward); emit AuctionRewardDistributed(_sequenceId, auctionResults[i].recipient, i, _reward); } } return bytes32(uint(drawId)); }
payable when interacting with ETH like for transfering ETH to other contractsFile: RemoteOwner.sol 63: function execute(address target, uint256 value, bytes calldata data) external returns (bytes memory) { ... (bool success, bytes memory returnData) = target.call{ value: value }(data);
ERC20 tokensSome IERC20 implementations (e.g UNI, COMP) may fail if the valued transferred is larger than uint96. Source
File: LiquidationRouter.sol 69: IERC20(_liquidationPair.tokenIn()).safeTransferFrom(
File: RngAuction.sol 182: IERC20(_feeToken).transferFrom(msg.sender, address(this), _requestFee);
File: VaultBooster.sol 173: _token.safeTransferFrom(msg.sender, address(this), _amount);
File: VaultBooster.sol 193: _token.transfer(msg.sender, _amount);
</details>File: VaultBooster.sol 226: IERC20(_tokenOut).safeTransfer(_account, _amountOut);
When a type is downcast to a smaller type, the higher order bits are truncated, effectively applying a modulo to the original value. Without any other checks, this wrapping will lead to unexpected behavior and bugs
File: RngAuction.sol 358: return uint64(block.timestamp);
File: RngRelayAuction.sol 139: uint64 _auctionElapsedSeconds = uint64(block.timestamp < _rngCompletedAt ? 0 : block.timestamp - _rngCompletedAt);
File: VaultBooster.sol 154: lastAccruedAt: uint48(block.timestamp) 154: uint48(block.timestamp) 163: uint48(block.timestamp) 224: uint48(block.timestamp) 252: uint48(block.timestamp) 224: _boosts[IERC20(_tokenOut)].lastAccruedAt = uint48(block.timestamp); 252: _boosts[_tokenOut].lastAccruedAt = uint48(block.timestamp); 273: uint256 totalSupply = twabController.getTotalSupplyTwabBetween(address(vault), uint32(boost.lastAccruedAt), uint32(block.timestamp));
Taking 0 as a valid argument in Solidity without checks can lead to severe security issues. A historical example is the infamous 0x0 address bug where numerous tokens were lost. This happens because 0 can be interpreted as an uninitialized address, leading to transfers to the 0x0 address, effectively burning tokens. Moreover, 0 as a denominator in division operations would cause a runtime exception. It's also often indicative of a logical error in the caller's code. It's important to always validate input and handle edge cases like 0 appropriately. Use require() statements to enforce conditions and provide clear error messages to facilitate debugging and safer code.
File: LiquidationPair.sol 289: return wrap(0);
File: LiquidationPair.sol 357: _initialPrice = wrap(0);
File: ContinuousGDA.sol 31: return SD59x18.wrap(0); 62: return SD59x18.wrap(0);
</details>File: RngAuctionRelayerRemoteOwner.sol 65: RemoteOwnerCallEncoder.encodeCalldata(address(_remoteRngAuctionRelayListener), 0, listenerCalldata)
block.timestamp is == to the compared variableThe condition does not revert when block.timestamp is equal to the compared > or < variable. For example, if there is a check for block.timestamp > _lastAuctionTime then there should be a check for cases where block.timestamp is == to _lastAuctionTime
File: LiquidationPair.sol 288: if (block.timestamp < _lastAuctionTime) {
File: RngRelayAuction.sol 139: uint64 _auctionElapsedSeconds = uint64(block.timestamp < _rngCompletedAt ? 0 : block.timestamp - _rngCompletedAt);
File: RngAuction.sol 347: function setNextRngService(RNGInterface _rngService) external onlyOwner { _setNextRngService(_rngService); }
File: RemoteOwner.sol 96: function setOriginChainOwner(address _newOriginChainOwner) external { _checkSender(); _setOriginChainOwner(_newOriginChainOwner); }
It is recommended to use pre-made headers for Solidity code layout and readability: https://github.com/transmissions11/headers
/*////////////////////////////////////////////////////////////// TESTING 123 //////////////////////////////////////////////////////////////*/
File: LiquidationPair.sol 5: LiquidationPair.sol
File: ContinuousGDA.sol 10: ContinuousGDA.sol
A check regarding whether the current value and the new value are the same should be added
File: RngAuction.sol 347: function setNextRngService(RNGInterface _rngService) external onlyOwner { _setNextRngService(_rngService); }
File: RemoteOwner.sol 96: function setOriginChainOwner(address _newOriginChainOwner) external { _checkSender(); _setOriginChainOwner(_newOriginChainOwner); }
The type of the variable is the same as the type to which the variable is being cast
File: VaultBooster.sol 192: IERC20(_token)
forge-std importforge-std is used for logging and debugging purposes and should be removed when not used for development.
File: VaultBooster.sol 4: import "forge-std/console2.sol";
error definitionNote that there may be cases where an error superficially appears to be used, but this is only because there are multiple definitions of the error in different files. In such cases, the error definition should be moved into a separate file. The instances below are the unused definitions.
File: RngAuction.sol error AuctionDurationGteSequencePeriod
Code base has an extensive use of named function calls, but it somehow missed one instance where this would be appropriate.
It should use named function calls on function call, as such:
library.exampleFunction{value: _data.amount.value}({ _id: _data.id, _amount: _data.amount.value, _token: _data.token, _example: "", _metadata: _data.metadata });
File: RemoteOwner.sol 67: (bool success, bytes memory returnData) = target.call{ value: value }(data);
Consider using descriptive constants or an enum instead of passing zero directly on function calls, as that might be error-prone, to fully describe the caller's intention.
File: RngAuctionRelayerRemoteOwner.sol 65: RemoteOwnerCallEncoder.encodeCalldata(address(_remoteRngAuctionRelayListener), 0, listenerCalldata)
#0 - c4-pre-sort
2023-08-08T23:41:55Z
raymondfam marked the issue as high quality report
#1 - c4-sponsor
2023-08-09T19:29:40Z
asselstine marked the issue as sponsor confirmed
#2 - c4-judge
2023-08-14T10:15:53Z
HickupHH3 marked the issue as grade-a
🌟 Selected for report: Rolezn
Also found by: 0xhex, 0xta, JCK, K42, Rageur, Raihan, ReyAdmirado, SAQ, SY_S, dharma09, hunter_w3b, petrichor, shamsulhaq123, wahedtalash77
459.0115 USDC - $459.01
| Issue | Contexts | Estimated Gas Saved | |
|---|---|---|---|
| GAS‑1 | Avoid emitting event on every iteration | 1 | 375 |
| GAS‑2 | Counting down in for statements is more gas efficient | 2 | 514 |
| GAS‑3 | Use assembly to write address storage values | 11 | 814 |
| GAS‑4 | Multiple accesses of a mapping/array should use a local variable cache | 8 | 640 |
| GAS‑5 | Remove forge-std import | 1 | 100 |
| GAS‑6 | The result of a function call should be cached rather than re-calling the function | 5 | 250 |
| GAS‑7 | Use do while loops instead of for loops | 2 | 8 |
| GAS‑8 | Use nested if and avoid multiple check combinations | 2 | 12 |
| GAS‑9 | Using XOR (^) and AND (&) bitwise equivalents | 6 | 78 |
Total: 44 contexts over 9 issues
Expensive operations should always try to be avoided within loops. Such operations include: reading/writing to storage, heavy calculations, external calls, and emitting events. In this instance, an event is being emitted every iteration. Events have a base cost of Glog (375 gas) per emit and Glogdata (8 gas) * number of bytes in event. We can avoid incurring those costs each iteration by emitting the event outside of the loop.
File: RngRelayAuction.sol 167: for (uint8 i = 0; i < _rewards.length; i++) { uint104 _reward = uint104(_rewards[i]); if (_reward > 0) { prizePool.withdrawReserve(auctionResults[i].recipient, _reward); emit AuctionRewardDistributed(_sequenceId, auctionResults[i].recipient, i, _reward); }
for statements is more gas efficientCounting down is more gas efficient than counting up because neither we are making zero variable to non-zero variable and also we will get gas refund in the last transaction when making non-zero to zero variable.
File: RngRelayAuction.sol 167: for (uint8 i = 0; i < _rewards.length; i++) {
File: RewardLib.sol 65: for (uint256 i; i < _auctionResultsLength; i++) {
contract GasTest is DSTest { Contract0 c0; Contract1 c1; function setUp() public { c0 = new Contract0(); c1 = new Contract1(); } function testGas() public { c0.AddNum(); c1.AddNum(); } } contract Contract0 { uint256 num = 3; function AddNum() public { uint256 _num = num; for(uint i=0;i<=9;i++){ _num = _num +1; } num = _num; } } contract Contract1 { uint256 num = 3; function AddNum() public { uint256 _num = num; for(uint i=9;i>=0;i--){ _num = _num +1; } num = _num; } }
| Contract0 contract | |||||
|---|---|---|---|---|---|
| Deployment Cost | Deployment Size | ||||
| 77011 | 311 | ||||
| Function Name | min | avg | median | max | # calls |
| AddNum | 7040 | 7040 | 7040 | 7040 | 1 |
| Contract1 contract | |||||
|---|---|---|---|---|---|
| Deployment Cost | Deployment Size | ||||
| 73811 | 295 | ||||
| Function Name | min | avg | median | max | # calls |
| AddNum | 3819 | 3819 | 3819 | 3819 | 1 |
assembly to write address storage valuesFile: LiquidationPair.sol 130: _lastNonZeroAmountIn = _initialAmountIn;
File: LiquidationPair.sol 131: _lastNonZeroAmountOut = _initialAmountOut;
File: LiquidationPair.sol 334: _lastNonZeroAmountIn = _amountInForPeriod;
File: LiquidationPair.sol 335: _lastNonZeroAmountOut = _amountOutForPeriod;
File: LiquidationPair.sol 342: _emissionRate = emissionRate_;
File: LiquidationRouter.sol 50: _liquidationPairFactory = liquidationPairFactory_;
File: RngAuction.sol 435: _nextRng = _newRng;
File: RngRelayAuction.sol 117: _auctionDurationSeconds = auctionDurationSeconds_;
File: RngRelayAuction.sol 145: _lastSequenceId = _sequenceId;
File: RemoteOwner.sol 57: _originChainId = originChainId_;
https://github.com/code-423n4/2023-08-pooltogether/tree/main/remote-owner/src/RemoteOwner.sol#L57
File: RemoteOwner.sol 106: _originChainOwner = _newOriginChainOwner;
https://github.com/code-423n4/2023-08-pooltogether/tree/main/remote-owner/src/RemoteOwner.sol#L106
</details>Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times saves ~42 gas per access due to not having to perform the same offset calculation every time. Help the Optimizer by saving a storage variable's reference instead of repeatedly fetching it
To help the optimizer,declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array. As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.
File: RngRelayAuction.sol 170: prizePool.withdrawReserve(auctionResults[i].recipient, _reward); 171: emit AuctionRewardDistributed(_sequenceId, auctionResults[i].recipient, i, _reward);
File: AddressRemapper.sol 33: if (_destinationAddress[_addr] == address(0)) { 36: return _destinationAddress[_addr];
File: VaultBooster.sol 223: _boosts[IERC20(_tokenOut)].available = amountAvailable.toUint144(); 224: _boosts[IERC20(_tokenOut)].lastAccruedAt = uint48(block.timestamp);
File: VaultBooster.sol 251: _boosts[_tokenOut].available = available.toUint144(); 252: _boosts[_tokenOut].lastAccruedAt = uint48(block.timestamp);
forge-stdIt's used to print the values of variables while running tests to help debug and see what's happening inside your contracts but since it's a development tool, it serves no purpose on mainnet.
Also, the remember to remove the usage of calls that use forge-std when removing of the import of forge-std.
File: VaultBooster.sol 4: import "forge-std/console2.sol";
External calls are expensive. Results of external function calls should be cached rather than call them multiple times. Consider caching the following:
File: RngAuction.sol 370: uint64 currentTime = _currentTime(); 382: uint64 currentTime = _currentTime(); 386: return (_currentTime() - sequenceOffset) % sequencePeriod;
File: RemoteOwner.sol 119: if (_fromChainId() != _originChainId) revert OriginChainIdUnsupported(_fromChainId()); 120: if (_msgSender() != address(_originChainOwner)) revert OriginSenderNotOwner(_msgSender());
https://github.com/code-423n4/2023-08-pooltogether/tree/main/remote-owner/src/RemoteOwner.sol#L119
https://github.com/code-423n4/2023-08-pooltogether/tree/main/remote-owner/src/RemoteOwner.sol#L120
A do while loop will cost less gas since the condition is not being checked for the first iteration.
File: RngRelayAuction.sol 131: function rngComplete for (uint8 i = 0; i < _rewards.length; i++) { uint104 _reward = uint104(_rewards[i]); if (_reward > 0) { prizePool.withdrawReserve(auctionResults[i].recipient, _reward); emit AuctionRewardDistributed(_sequenceId, auctionResults[i].recipient, i, _reward); }
File: RewardLib.sol 58: function rewards for (uint256 i; i < _auctionResultsLength; i++) { _rewards[i] = reward(_auctionResults[i], remainingReserve); remainingReserve = remainingReserve - _rewards[i]; }
if and avoid multiple check combinationsUsing nested if, is cheaper than using && multiple check combinations. There are more advantages, such as easier to read code and better coverage reports.
File: LiquidationPair.sol 332: if (_amountInForPeriod > 0 && _amountOutForPeriod > 0) {
File: RngAuction.sol 179: if (_feeToken != address(0) && _requestFee > 0) {
contract GasTest is DSTest { Contract0 c0; Contract1 c1; function setUp() public { c0 = new Contract0(); c1 = new Contract1(); } function testGas() public { c0.checkAge(19); c1.checkAgeOptimized(19); } } contract Contract0 { function checkAge(uint8 _age) public returns(string memory){ if(_age>18 && _age<22){ return "Eligible"; } } } contract Contract1 { function checkAgeOptimized(uint8 _age) public returns(string memory){ if(_age>18){ if(_age<22){ return "Eligible"; } } } }
| Contract0 contract | |||||
|---|---|---|---|---|---|
| Deployment Cost | Deployment Size | ||||
| 76923 | 416 | ||||
| Function Name | min | avg | median | max | # calls |
| checkAge | 651 | 651 | 651 | 651 | 1 |
| Contract1 contract | |||||
|---|---|---|---|---|---|
| Deployment Cost | Deployment Size | ||||
| 76323 | 413 | ||||
| Function Name | min | avg | median | max | # calls |
| checkAgeOptimized | 645 | 645 | 645 | 645 | 1 |
Given 4 variables a, b, c and d represented as such:
0 0 0 0 0 1 1 0 <- a 0 1 1 0 0 1 1 0 <- b 0 0 0 0 0 0 0 0 <- c 1 1 1 1 1 1 1 1 <- d
To have a == b means that every 0 and 1 match on both variables. Meaning that a XOR (operator ^) would evaluate to 0 ((a ^ b) == 0), as it excludes by definition any equalities.Now, if a != b, this means that there’s at least somewhere a 1 and a 0 not matching between a and b, making (a ^ b) != 0.Both formulas are logically equivalent and using the XOR bitwise operator costs actually the same amount of gas.However, it is much cheaper to use the bitwise OR operator (|) than comparing the truthy or falsy values.These are logically equivalent too, as the OR bitwise operator (|) would result in a 1 somewhere if any value is not 0 between the XOR (^) statements, meaning if any XOR (^) statement verifies that its arguments are different.
File: LiquidationPair.sol 298: if (_amountOut == 0) { 314: if (purchasePrice == 0) {
File: RewardLib.sol 83: if (_auctionResult.recipient == address(0)) return 0; 84: if (_reserve == 0) return 0;
File: VaultBooster.sol 266: if (deltaTime == 0) {
File: RemoteOwner.sol 104: if (_newOriginChainOwner == address(0)) revert OriginChainOwnerZeroAddress();
https://github.com/code-423n4/2023-08-pooltogether/tree/main/remote-owner/src/RemoteOwner.sol#L104
contract GasTest is DSTest { Contract0 c0; Contract1 c1; function setUp() public { c0 = new Contract0(); c1 = new Contract1(); } function testGas() public { c0.not_optimized(1,2); c1.optimized(1,2); } } contract Contract0 { function not_optimized(uint8 a,uint8 b) public returns(bool){ return ((a==1) || (b==1)); } } contract Contract1 { function optimized(uint8 a,uint8 b) public returns(bool){ return ((a ^ 1) & (b ^ 1)) == 0; } }
| Contract0 contract | |||||
|---|---|---|---|---|---|
| Deployment Cost | Deployment Size | ||||
| 46099 | 261 | ||||
| Function Name | min | avg | median | max | # calls |
| not_optimized | 456 | 456 | 456 | 456 | 1 |
| Contract1 contract | |||||
|---|---|---|---|---|---|
| Deployment Cost | Deployment Size | ||||
| 42493 | 243 | ||||
| Function Name | min | avg | median | max | # calls |
| optimized | 430 | 430 | 430 | 430 | 1 |
#0 - c4-pre-sort
2023-08-09T00:01:25Z
raymondfam marked the issue as high quality report
#1 - c4-sponsor
2023-08-09T19:30:24Z
asselstine marked the issue as sponsor confirmed
#2 - c4-judge
2023-08-14T11:07:41Z
HickupHH3 marked the issue as selected for report
#3 - c4-judge
2023-08-14T11:07:45Z
HickupHH3 marked the issue as grade-a