Paladin - Warden Pledges contest - indijanc's results

Lines of code

https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L18

Vulnerability details

Impact

2 step ownership change is a more secure way of changing contract owners where the next owner must accept ownership. A mistake on ownership change would render the contract unable to add/remove reward tokens, change chest contract and doing other upgrades.

This is normally not a high issue and depends on the individual project and use case, but I consider it a medium in this case because the documentation mentions 2-step ownership change, it is also implemented and prepared in Owner.sol but is never used.

Proof of Concept

This simple failing test will showcase that WardenPledge.sol is not using 2-step ownership change:

diff --git a/test/wardenPledge.test.ts b/test/wardenPledge.test.ts
index 5167d0d..44b49a5 100644
--- a/test/wardenPledge.test.ts
+++ b/test/wardenPledge.test.ts
@@ -29,6 +29,7 @@ import {
     HOLDERS,
     AMOUNTS
 } from "./utils/constant"
+import exp from "constants";

 chai.use(solidity);
 const { expect } = chai;
@@ -230,6 +231,12 @@ describe('Warden Pledge contract tests', () => {

         });

+        it(' should use 2-step fow WardenPledge ownership change', async () => {
+            await wardenPledge.connect(admin).transferOwnership(delegator1.address)
+            // 2 step ownershipe change should keep old owner until new owner accepts
+            await expect(await wardenPledge.owner()).to.be.eq(admin.address)
+        });
+
     });

     describe('addMultipleRewardToken', async () => {

Tools Used

Manual review

Recommended Mitigation Steps

I recommend using the implemented and already prepared Owner.sol to use 2-step ownerrship change. Derive WardenPledge from Owner and you will find the previously failing test to succeed:

diff --git a/contracts/WardenPledge.sol b/contracts/WardenPledge.sol
index beb990d..dab9ac6 100644
--- a/contracts/WardenPledge.sol
+++ b/contracts/WardenPledge.sol
@@ -15,7 +15,7 @@ import "./utils/Errors.sol";
 /*
     Delegation market (Pledge version) based on Curve Boost V2 contract
 */
-contract WardenPledge is Ownable, Pausable, ReentrancyGuard {
+contract WardenPledge is Owner, Pausable, ReentrancyGuard {
     using SafeERC20 for IERC20;

     // Constants :

Gas optimizations

Redundant variables

creator in createPledge() can be replaced with msg.sender

WardenPledge.sol L308 Redundant variable creator can be removed - use msg.sender instead. This slightly reduces contract size and interaction cost on createPledge() Using the project tests we see the following gas improvement after removal:

creator in retrievePledgeRewards() can be removed

WardenPledge.sol L458 Redundant variable creator can be removed. This slightly reduces contract size and interaction cost on retrievePledgeRewards(). Use pledgeOwner[pledgeId] in the if statement instead. Using the project tests we see the following improvement after removal:

creator in closePledge() can be replaced with msg.sender

WardenPledge.sol L490 Redundant variable creator can be removed - use msg.sender instead. This slightly reduces contract size and interaction cost on closePledge() Using the project tests we see the following gas improvement after removal:

Redundant variables oldMinTarget and oldfee in update methods

WardenPledge.sol L614 WardenPledge.sol L627 By emitting before assignment you can remove the redundant variable. There is a slight increase in deployment cost however. Using the project tests we see the following gas improvement after removal of both variables:

Redundant operations

Redundant add operation in createPledge()

WardenPledge.sol L340 Redundant addition costs more gas than assignment. pledgeAvailableRewardAmounts of the newly created pledge will always start with 0, so the add sign doesn't make much sense and increases gos cast on each pledge creation. Using the project tests we see the following improvement by removing the addition:

Redundant return statement in recoverERC20()

WardenPledge.sol L660 The return statement in recoverERC20() always returns true. Seems redundant, the function and transfer of funds will revert on failure, or pass otherwise. Removing return statement saves some gas. Using the project tests here's the gas improvement after removing the return statement: