search

code-423n4 / 2022-07-swivel-findings

0 stars 1 forks source link

QA Report #62

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Admin change should be 2 step process

Contract: https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L428

Issue: It is observed that current owner can designate a new owner by calling setAdmin function at Swivel.sol#L428 in a single step. This is a risky operation which could lead to access loss for all Admin privilleged functions

Steps:

  1. Current Admin accidentally sets the new Admin incorrectly
  2. Since new Admin was set incorrectly, all functions requiring Admin privillege cannot be accessed like withdraw, authRedeemZcToken etc

Recommendation: It is recommended to move to a two step Admin change.

a. First function should decide the pending owner b. Second function which is only callable by pending owner should actually change the Admin

Recovered address should not be zero

Contract: https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L693

Issue: ecrecover returns empty (0x0) address when the signature is invalid. validOrderHash indirectly makes use of ecrecover while checking Signatures and missing the zero address checking

Steps:

  1. Assume validOrderHash function is called with an invalid signature.
  2. This causes recovered to be address 0
 address recovered = Sig.recover(Hash.message(domain, hash), c);
  1. Now if order maker is address 0 (not sure even if it is possible) then this order will be considered valid which is incorrectly (Since o.maker and recovered both are zero address)
if (o.maker != recovered) { revert Exception(4, 0, 0, o.maker, recovered); }

    return hash;

Recommendation: Add zero address check

if (recovered!=address(0) && o.maker != recovered) { revert Exception(4, 0, 0, o.maker, recovered); }

Existing timelock gets delayed

Issue: If admin accidentally calls scheduleApproval twice on same address then Timelock will get extended even though address for approval is same

Steps:

  1. Admin scheduleApproval for address Admin so that address A will be approved after 3 days
  2. After 2 days, accidentally Admin again scheduleApproval for address Admin so that address A will be approved after 3 days
  3. This means the approval will now take 5 days instead of 3

Recommendation: Add a check to see if approval is already requested for a given address

require(approvals[e]==0, "Already pending approval");