Platform: Code4rena
Start Date: 27/10/2022
Pot Size: $33,500 USDC
Total HM: 8
Participants: 96
Period: 3 days
Judge: kirk-baird
Total Solo HM: 1
Id: 176
League: ETH
Rank: 27/96
Findings: 2
Award: $180.63
π Selected for report: 1
π Solo Findings: 0
π Selected for report: robee
Also found by: 0x007, 0x1f8b, 0x52, 0xDjango, 0xNazgul, 0xSmartContract, 8olidity, Awesome, B2, Bnke0x0, Chom, Diana, Dravee, JTJabba, Jeiwan, Josiah, Lambda, Mathieu, Picodes, RaoulSchaffranek, RaymondFam, RedOneN, ReyAdmirado, Rolezn, Ruhum, Sm4rty, Tricko, Trust, Waze, __141345__, a12jmx, adriro, ajtra, brgltd, c3phas, carlitox477, cccz, ch0bu, chaduke, chrisdior4, corerouter, cryptonue, csanuragjain, ctf_sec, cylzxje, delfin454000, dic0de, djxploit, horsefacts, imare, jayphbee, jwood, ktg, ladboy233, leosathya, lukris02, minhtrng, neko_nyaa, oyc_109, pashov, peritoflores, rbserver, rvierdiiev, shark, tnevler, yixxas
19.6449 USDC - $19.64
File: /contracts/WardenPledge.sol 137: votingEscrow = IVotingEscrow(_votingEscrow); 138: delegationBoost = IBoostV2(_delegationBoost); 140: chestAddress = _chestAddress;
There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.
Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name.
File: /contracts/WardenPledge.sol //@audit: 500 626: if(newFee > 500) revert Errors.InvalidValue();
File: /contracts/WardenPledge.sol //@audit: **balacne** instead of **balance* 292: * @param targetVotes Maximum taget of votes to have (own balacne + delegation) for the receiver
File: /contracts/WardenPledge.sol //@audit: **ot** instead of **to** 295: * @param maxTotalRewardAmount Maximum total reward amount allowed ot be pulled by this contract 296: * @param maxFeeAmount Maximum feeamount allowed ot be pulled by this contract 365: * @param maxTotalRewardAmount Maximum added total reward amount allowed ot be pulled by this contract 366: * @param maxFeeAmount Maximum fee amount allowed ot be pulled by this contract 411: * @param maxTotalRewardAmount Maximum added total reward amount allowed ot be pulled by this contract 412: * @param maxFeeAmount Maximum fee amount allowed ot be pulled by this contract
File: /contracts/WardenPledge.sol //@audit: **Minmum** instead of **Minimum** 523: * @param minRewardPerSecond Minmum amount of reward per vote per second for the token
File: /contracts/WardenPledge.sol //@audit: **Minmum** instead of **Minimum** 539: * @param minRewardsPerSecond Minmum amount of reward per vote per second for each token in the list
File: /contracts/WardenPledge.sol //@audit: **Minmum** instead of **Minimum** 558: * @param minRewardPerSecond Minmum amount of reward per vote per second for the token
File: /contracts/WardenPledge.sol //@audit: **Minmum** instead of **Minimum** 568: * @param minRewardPerSecond Minmum amount of reward per vote per second for the token
File: /contracts/WardenPledge.sol //@audit: Platfrom instead of Platform 621: * @notice Updates the Platfrom fees BPS ratio 622: * @dev Updates the Platfrom fees BPS ratio
File: /contracts/WardenPledge.sol //@audit: **tof** instead of **of** 650: * @param token Address tof the EC2O token
File: /contracts/WardenPledge.sol //@audit: Missing @param _chestAddress 125: /** 126: * @dev Creates the contract, set the given base parameters 127: * @param _votingEscrow address of the voting token to delegate 128: * @param _delegationBoost address of the contract handling delegation 129: * @param _minTargetVotes min amount of veToken to target in a Pledge 130: */ 131: constructor( 132: address _votingEscrow, 133: address _delegationBoost, 134: address _chestAddress, 135: uint256 _minTargetVotes 136: ) {
indexed
fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event
should use three indexed
fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
File: /contracts/WardenPledge.sol 85: event NewPledge( 86: address creator, 87: address receiver, 88: address rewardToken, 89: uint256 targetVotes, 90: uint256 rewardPerVote, 91: uint256 endTimestamp 92: ); 115: event ChestUpdated(address oldChest, address newChest); 116: /** @notice Event emitted when xx */ 117: event PlatformFeeUpdated(uint256 oldfee, uint256 newFee); 118: /** @notice Event emitted when xx */ 119: event MinTargetUpdated(uint256 oldMinTarget, uint256 newMinTargetVotes);
This is a best-practice to protect against reentrancy in other modifiers
File: /contracts/WardenPledge.sol 195: function pledge(uint256 pledgeId, uint256 amount, uint256 endTimestamp) external whenNotPaused nonReentrant { 206: function pledgePercent(uint256 pledgeId, uint256 percent, uint256 endTimestamp) external whenNotPaused nonReentrant { 299: function createPledge( 300: address receiver, 301: address rewardToken, 302: uint256 targetVotes, 303: uint256 rewardPerVote, // reward/veToken/second 304: uint256 endTimestamp, 305: uint256 maxTotalRewardAmount, 306: uint256 maxFeeAmount 307: ) external whenNotPaused nonReentrant returns(uint256){ 368: function extendPledge( 369: uint256 pledgeId, 370: uint256 newEndTimestamp, 371: uint256 maxTotalRewardAmount, 372: uint256 maxFeeAmount 373: ) external whenNotPaused nonReentrant { 414: function increasePledgeRewardPerVote( 415: uint256 pledgeId, 416: uint256 newRewardPerVote, 417: uint256 maxTotalRewardAmount, 418: uint256 maxFeeAmount 419: ) external whenNotPaused nonReentrant { 456: function retrievePledgeRewards(uint256 pledgeId, address receiver) external whenNotPaused nonReentrant { 488: function closePledge(uint256 pledgeId, address receiver) external whenNotPaused nonReentrant {
Some constructs deviate from this recommended best-practice: External/public functions are mixed with internal/private ones Ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier.
Functions should be grouped according to their visibility and ordered:-
Our code does not seem to follow this recommendations . We have public functions declared before external functions
File: /contracts/WardenPledge.sol 153: function pledgesIndex() public view returns(uint256){ 163: function getUserPledges(address user) external view returns(uint256[] memory){
Internal functions are also mixed with external functions rather than be ordered accordingly
Recommendation: Consider adopting recommended best-practice for code structure and layout. See Documentation
#0 - c4-judge
2022-11-12T00:58:07Z
kirk-baird marked the issue as grade-b
π Selected for report: c3phas
Also found by: 0x1f8b, 0xNazgul, 0xRoxas, 0xSmartContract, 0xbepresent, Amithuddar, Awesome, B2, Bnke0x0, Dravee, KoKo, Mathieu, Picodes, RaymondFam, RedOneN, ReyAdmirado, RockingMiles, Ruhum, SadBase, SooYa, Waze, __141345__, adriro, ajtra, ballx, carlitox477, ch0bu, cylzxje, djxploit, durianSausage, emrekocak, erictee, gogo, halden, horsefacts, imare, indijanc, karanctf, leosathya, lukris02, neko_nyaa, oyc_109, peiw, sakman, shark, skyle, tnevler
160.9924 USDC - $160.99
NB: Some functions have been truncated where neccessary to just show affected parts of the code.
Throught the report some places might be denoted with audit tags to show the actual place affected.
Use immutable if you want to assign a permanent value at construction. Use constants if you already know the permanent value. Both get directly embedded in bytecode, saving SLOAD. Variables only set in the constructor and never edited afterwards should be marked as immutable, as it would avoid the expensive storage-writing operation in the constructor (around 20 000 gas per variable) and replace the expensive storage-reading operations (around 2100 gas per reading) to a less expensive value reading (3 gas)
File: /contracts/WardenPledge.sol 60: IVotingEscrow public votingEscrow;
File: /contracts/WardenPledge.sol 62: IBoostV2 public delegationBoost;
File: /contracts/WardenPledge.sol 79: uint256 public minDelegationTime = 1 weeks;
diff --git a/contracts/WardenPledge.sol b/contracts/WardenPledge.sol index beb990d..642a848 100644 --- a/contracts/WardenPledge.sol +++ b/contracts/WardenPledge.sol @@ -76,7 +76,7 @@ contract WardenPledge is Ownable, Pausable, ReentrancyGuard { uint256 public minTargetVotes; /** @notice Minimum delegation time, taken from veBoost contract */ - uint256 public minDelegationTime = 1 weeks; + uint256 public constant minDelegationTime = 1 weeks;
The code can be optimizhttps://github.com/code-423n4/2022-08-fiatdao-findings/blob/main/data/GalloDaSballo-G.mded by minimizing the number of SLOADs.
SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.
NB: *Some functions have been truncated where neccessary to just show affected parts of the code
File: /contracts/WardenPledge.sol 222: function _pledge(uint256 pledgeId, address user, uint256 amount, uint256 endTimestamp) internal { 240: delegationBoost.checkpoint_user(user); //@audit: 1st SLOAD 241: if(delegationBoost.allowance(user, address(this)) < amount) revert Errors.InsufficientAllowance(); //@audit: 2nd SLOAD 242: if(delegationBoost.delegable_balance(user) < amount) revert Errors.CannotDelegate(); //@audit: 3rd SLOAD 245: if(delegationBoost.adjusted_balance_of(pledgeParams.receiver) + amount > pledgeParams.targetVotes) revert Errors.TargetVotesOverflow(); //@audit: 4th SLOAD 248: delegationBoost.boost( //@audit: 5th SLOAD pledgeParams.receiver, amount, endTimestamp, user );
diff --git a/contracts/WardenPledge.sol b/contracts/WardenPledge.sol index beb990d..5b3d1bd 100644 --- a/contracts/WardenPledge.sol +++ b/contracts/WardenPledge.sol @@ -237,15 +237,16 @@ contract WardenPledge is Ownable, Pausable, ReentrancyGuard { uint256 boostDuration = endTimestamp - block.timestamp; // Check that the user has enough boost delegation available & set the correct allowance to this contract - delegationBoost.checkpoint_user(user); - if(delegationBoost.allowance(user, address(this)) < amount) revert Errors.InsufficientAllowance(); - if(delegationBoost.delegable_balance(user) < amount) revert Errors.CannotDelegate(); + IBoostV2 _delegationBoost = delegationBoost; + _delegationBoost.checkpoint_user(user); + if(_delegationBoost.allowance(user, address(this)) < amount) revert Errors.InsufficientAllowance(); + if(_delegationBoost.delegable_balance(user) < amount) revert Errors.CannotDelegate(); // Check that this will not go over the Pledge target of votes - if(delegationBoost.adjusted_balance_of(pledgeParams.receiver) + amount > pledgeParams.targetVotes) revert Errors.TargetVotesOverflow(); + if(_delegationBoost.adjusted_balance_of(pledgeParams.receiver) + amount > pledgeParams.targetVotes) revert Errors.TargetVotesOverflow(); // Creates the DelegationBoost - delegationBoost.boost( + _delegationBoost.boost( pledgeParams.receiver, amount, endTimestamp,
File: /contracts/WardenPledge.sol function _pledge(uint256 pledgeId, address user, uint256 amount, uint256 endTimestamp) internal { 267: if(rewardAmount > pledgeAvailableRewardAmounts[pledgeId]) revert Errors.RewardsBalanceTooLow(); //@audit: 1st access 268: pledgeAvailableRewardAmounts[pledgeId] -= rewardAmount; //@audit: 2nd access
diff --git a/contracts/WardenPledge.sol b/contracts/WardenPledge.sol index beb990d..2bb2cd2 100644 --- a/contracts/WardenPledge.sol +++ b/contracts/WardenPledge.sol @@ -263,9 +263,10 @@ contract WardenPledge is Ownable, Pausable, ReentrancyGuard { uint256 totalDelegatedAmount = ((bias * boostDuration) + bias) / 2; // Then we can calculate the total amount of rewards for this Boost uint256 rewardAmount = (totalDelegatedAmount * pledgeParams.rewardPerVote) / UNIT; + uint _pledgeAvailableRewardAmounts = pledgeAvailableRewardAmounts[pledgeId]; - if(rewardAmount > pledgeAvailableRewardAmounts[pledgeId]) revert Errors.RewardsBalanceTooLow(); - pledgeAvailableRewardAmounts[pledgeId] -= rewardAmount; + if(rewardAmount > _pledgeAvailableRewardAmounts) revert Errors.RewardsBalanceTooLow(); + pledgeAvailableRewardAmounts[pledgeId] = _pledgeAvailableRewardAmounts - rewardAmount; // Send the rewards to the user IERC20(pledgeParams.rewardToken).safeTransfer(user, rewardAmount);
File: /contracts/WardenPledge.sol 299: function createPledge( 312: if(minAmountRewardToken[rewardToken] == 0) revert Errors.TokenNotWhitelisted(); //@audit: 1st access 313: if(rewardPerVote < minAmountRewardToken[rewardToken]) revert Errors.RewardPerVoteTooLow(); //@audit: 2nd access
diff --git a/contracts/WardenPledge.sol b/contracts/WardenPledge.sol index beb990d..247e5f8 100644 --- a/contracts/WardenPledge.sol +++ b/contracts/WardenPledge.sol @@ -309,8 +309,9 @@ contract WardenPledge is Ownable, Pausable, ReentrancyGuard { if(receiver == address(0) || rewardToken == address(0)) revert Errors.ZeroAddress(); if(targetVotes < minTargetVotes) revert Errors.TargetVoteUnderMin(); - if(minAmountRewardToken[rewardToken] == 0) revert Errors.TokenNotWhitelisted(); - if(rewardPerVote < minAmountRewardToken[rewardToken]) revert Errors.RewardPerVoteTooLow(); + uint256 _minAmountRewardToken = minAmountRewardToken[rewardToken] + if(_minAmountRewardToken == 0) revert Errors.TokenNotWhitelisted(); + if(rewardPerVote < _minAmountRewardToken) revert Errors.RewardPerVoteTooLow(); if(endTimestamp == 0) revert Errors.NullEndTimestamp(); if(endTimestamp != _getRoundedTimestamp(endTimestamp)) revert Errors.InvalidEndTimestamp();
Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas) in a function that may ultimately revert in the unhappy case.
File: /contracts/WardenPledge.sol 222: function _pledge(uint256 pledgeId, address user, uint256 amount, uint256 endTimestamp) internal { 223: if(pledgeId >= pledgesIndex()) revert Errors.InvalidPledgeID(); 224: if(amount == 0) revert Errors.NullValue();
diff --git a/contracts/WardenPledge.sol b/contracts/WardenPledge.sol index beb990d..dfd3ff4 100644 --- a/contracts/WardenPledge.sol +++ b/contracts/WardenPledge.sol @@ -220,8 +220,9 @@ contract WardenPledge is Ownable, Pausable, ReentrancyGuard { * @param endTimestamp End of delegation */ function _pledge(uint256 pledgeId, address user, uint256 amount, uint256 endTimestamp) internal { - if(pledgeId >= pledgesIndex()) revert Errors.InvalidPledgeID(); if(amount == 0) revert Errors.NullValue(); + if(pledgeId >= pledgesIndex()) revert Errors.InvalidPledgeID(); // Load Pledge parameters & check the Pledge is still active Pledge memory pledgeParams = pledges[pledgeId];
File: /contracts/WardenPledge.sol 456: function retrievePledgeRewards(uint256 pledgeId, address receiver) external whenNotPaused nonReentrant { 457: if(pledgeId >= pledgesIndex()) revert Errors.InvalidPledgeID(); 458: address creator = pledgeOwner[pledgeId]; 459: if(msg.sender != creator) revert Errors.NotPledgeCreator(); 460: if(receiver == address(0)) revert Errors.ZeroAddress();
diff --git a/contracts/WardenPledge.sol b/contracts/WardenPledge.sol index beb990d..9c82ad9 100644 --- a/contracts/WardenPledge.sol +++ b/contracts/WardenPledge.sol @@ -454,10 +454,11 @@ contract WardenPledge is Ownable, Pausable, ReentrancyGuard { * @param receiver Address to receive the remaining rewards */ function retrievePledgeRewards(uint256 pledgeId, address receiver) external whenNotPaused nonReentrant { + if(receiver == address(0)) revert Errors.ZeroAddress(); if(pledgeId >= pledgesIndex()) revert Errors.InvalidPledgeID(); address creator = pledgeOwner[pledgeId]; if(msg.sender != creator) revert Errors.NotPledgeCreator(); - if(receiver == address(0)) revert Errors.ZeroAddress(); + Pledge storage pledgeParams = pledges[pledgeId]; if(pledgeParams.endTimestamp > block.timestamp) revert Errors.PledgeNotExpired();
File: /contracts/WardenPledge.sol 488: function closePledge(uint256 pledgeId, address receiver) external whenNotPaused nonReentrant { 489: if(pledgeId >= pledgesIndex()) revert Errors.InvalidPledgeID(); 490: address creator = pledgeOwner[pledgeId]; 491: if(msg.sender != creator) revert Errors.NotPledgeCreator(); 492: if(receiver == address(0)) revert Errors.ZeroAddress();
diff --git a/contracts/WardenPledge.sol b/contracts/WardenPledge.sol index beb990d..c06f2ee 100644 --- a/contracts/WardenPledge.sol +++ b/contracts/WardenPledge.sol @@ -486,10 +486,11 @@ contract WardenPledge is Ownable, Pausable, ReentrancyGuard { * @param receiver Address to receive the remaining rewards */ function closePledge(uint256 pledgeId, address receiver) external whenNotPaused nonReentrant { + if(receiver == address(0)) revert Errors.ZeroAddress(); if(pledgeId >= pledgesIndex()) revert Errors.InvalidPledgeID(); address creator = pledgeOwner[pledgeId]; if(msg.sender != creator) revert Errors.NotPledgeCreator(); - if(receiver == address(0)) revert Errors.ZeroAddress();
File: /contracts/WardenPledge.sol 525: function _addRewardToken(address token, uint256 minRewardPerSecond) internal { 526: if(minAmountRewardToken[token] != 0) revert Errors.AlreadyAllowedToken(); 527: if(token == address(0)) revert Errors.ZeroAddress(); 528: if(minRewardPerSecond == 0) revert Errors.NullValue();
diff --git a/contracts/WardenPledge.sol b/contracts/WardenPledge.sol index beb990d..71d0087 100644 --- a/contracts/WardenPledge.sol +++ b/contracts/WardenPledge.sol @@ -523,10 +523,10 @@ contract WardenPledge is Ownable, Pausable, ReentrancyGuard { * @param minRewardPerSecond Minmum amount of reward per vote per second for the token */ function _addRewardToken(address token, uint256 minRewardPerSecond) internal { - if(minAmountRewardToken[token] != 0) revert Errors.AlreadyAllowedToken(); if(token == address(0)) revert Errors.ZeroAddress(); if(minRewardPerSecond == 0) revert Errors.NullValue(); + if(minAmountRewardToken[token] != 0) revert Errors.AlreadyAllowedToken(); + minAmountRewardToken[token] = minRewardPerSecond; emit NewRewardToken(token, minRewardPerSecond);
When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct
File: /contracts/WardenPledge.sol 227: Pledge memory pledgeParams = pledges[pledgeId];
Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isnβt possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block see resource
File: /contracts/WardenPledge.sol 268: pledgeAvailableRewardAmounts[pledgeId] -= rewardAmount;
The operation pledgeAvailableRewardAmounts[pledgeId] -= rewardAmount;
cannot underflow due to the check on Line 267 that ensures that pledgeAvailableRewardAmounts[pledgeId]
is greater than rewardAmount
before perfoming the arithmetic operation
#0 - c4-judge
2022-11-12T00:57:21Z
kirk-baird marked the issue as grade-a
#1 - c4-judge
2022-11-12T00:57:49Z
kirk-baird marked the issue as selected for report