Swivel v3 contest - jonatascm's results

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Swivel/Swivel.sol#L507

Vulnerability details

Impact

feenominators supposed to be updated using the array of indexes i not the current position of array length d

Proof of Concept

The function updates the fenominators[x] instead of feenominators[i[x]]

for (uint256 x; x < len;) {
  if (d[x] < MIN_FEENOMINATOR) { revert Exception(18, uint256(d[x]), 0, address(0), address(0)); }
  //Not updating the position i[x] of feenominators variable
  feenominators[x] = d[x];
  emit SetFee(i[x], d[x]);

  unchecked {
    x++;
  }
}

Recommended Mitigation Steps

To fix this issue following this code:

for (uint256 x; x < len;) {
  if (d[x] < MIN_FEENOMINATOR) { revert Exception(18, uint256(d[x]), 0, address(0), address(0)); }
- feenominators[x] = d[x];
+ feenominators[i[x]] = d[x];
  emit SetFee(i[x], d[x]);

  unchecked {
    x++;
  }
}

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Tokens/ZcToken.sol#L131-L136 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Tokens/ZcToken.sol#L110-L118

Vulnerability details

Impact

If any holder wants to withdraw ZcToken in behalf of any user, it can't be done because of the verification in withdraw function.

Proof of Concept

Following the logic in the code:

+ //Skip this logic because it is someone else executing
if (holder == msg.sender) {...}
else {
    uint256 allowed = allowance[holder][msg.sender];
+		//If the holders allowed more then previewAmount reverts
    if (allowed >= previewAmount) {
        revert Approvals(allowed, previewAmount);
    }
+. //Here: allowance[holder][msg.sender] < previewAmount

+  //But if the allowed is less than previewAmount it will breaks here
    allowance[holder][msg.sender] -= previewAmount;
    redeemer.authRedeem(protocol, underlying, maturity, holder, receiver, previewAmount); 
    return previewAmount;
}

Recommended Mitigation Steps

To fix this issue following this code:

- if (allowed >= previewAmount) {
+ if (allowed < previewAmount) {
    revert Approvals(allowed, previewAmount);
}

...

- if (allowed >= principalAmount) {
+ if (allowed < principalAmount) {
     revert Approvals(allowed, principalAmount); 
}

create can be executed before setMarketPlace by mistake, creating a ZcToken with wrong marketPlace

Target codebase

Creator.sol#L54-L61

Vulnerability details

Impact

Before deploy the Creator contract the owner can call the create function before setMarketPlace that don't check the marketPlace and by mistake can create a invalid ZcToken

Proof of ConceptThe function updates the fenominators[x] instead of feenominators[i[x]]

Recommended Mitigation Steps

Should remove the function setMarketPlace and add marketPlace to constructor.

- address public marketPlace;
+ address public immutable marketPlace;

constructor(address m) {
  admin = msg.sender;
+ marketPlace = m;
}

- function setMarketPlace(address m) external authorized(admin) returns (bool) {
-  if (marketPlace != address(0)) {
-    revert Exception(33, 0, 0, marketPlace, address(0)); 
-  }
-  marketPlace = m;
-  return true;
- }

Natspec issue

Target codebase

VaultTracker.sol#L31

Swivel.sol#L615

Vulnerability details

Impact

Natspec is incorrect or missing

Recommended Mitigation Steps

Should fix the following natspec:

In Swivel.sol:
- /// @notice p Protocol Enum value associated with this market pair
+ /// @param p Protocol Enum value associated with this market pair

In VaultTracker

+ /// @param p Protocol Enum value associated with this market pair
  /// @param m Maturity timestamp associated with this vault
  /// @param c Compounding Token address associated with this vault
  /// @param s Address of the deployed swivel contract
  constructor(uint8 p, uint256 m, address c, address s) {