Swivel v3 contest - 8olidity's results

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/main/Tokens/ZcToken.sol#L115

Vulnerability details

Impact

Zctoken.sol logical error?

Proof of Concept

uint256 allowed = allowance[holder][msg.sender];
if (allowed >= previewAmount) {
    revert Approvals(allowed, previewAmount);
}
allowance[holder][msg.sender] -= previewAmount;
redeemer.authRedeem(protocol, underlying, maturity, holder, receiver, previewAmount); 
return previewAmount;

Here allowance[holder][msg.sender] should be smaller than previewAmount, but if the smaller is smaller, an error should be reported.

File: Creator/ZcToken.sol:
  110          else {
  111:             uint256 allowed = allowance[holder][msg.sender];
  112              if (allowed >= previewAmount) {

  114              }
  115:             allowance[holder][msg.sender] -= previewAmount;
  116              redeemer.authRedeem(protocol, underlying, maturity, holder, receiver, previewAmount); 

  131          else {
  132:             uint256 allowed = allowance[holder][msg.sender];
  133              if (allowed >= principalAmount) { revert Approvals(allowed, principalAmount); }
  134:             allowance[holder][msg.sender] -= principalAmount;  
  135              return redeemer.authRedeem(protocol, underlying, maturity, holder, receiver, principalAmount);

File: Tokens/ZcToken.sol:
  110          else {
  111:             uint256 allowed = allowance[holder][msg.sender];
  112              if (allowed >= previewAmount) {

  114              }
  115:             allowance[holder][msg.sender] -= previewAmount;
  116              redeemer.authRedeem(protocol, underlying, maturity, holder, receiver, previewAmount); 

  131          else {
  132:             uint256 allowed = allowance[holder][msg.sender];
  133              if (allowed >= principalAmount) { revert Approvals(allowed, principalAmount); }
  134:             allowance[holder][msg.sender] -= principalAmount;  
  135              return redeemer.authRedeem(protocol, underlying, maturity, holder, receiver, principalAmount);

Tools Used

vscode

Recommended Mitigation Steps

Is there a problem with my judgment? If there is a problem with my judgment, please send me an email and let me know, so I can learn it

OPEN TODOS

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

File: Swivel/Swivel.sol:
   32    /// @dev address of a deployed Aave contract implementing IAave
   33:   address public aaveAddr; // TODO immutable?
   34  

  119      
  120:     // TODO cheaper to assign amount here or keep the ADD?
  121      filled[hash] += a;

  156  
  157:     // TODO assign amount or keep the ADD?
  158      filled[hash] += a;

  191  
  192:     // TODO assign amount or keep the ADD?
  193      filled[hash] += a;

  220  
  221:     // TODO assign amount or keep ADD?
  222      filled[hash] += a;

  285  
  286:     // TODO assign amount or keep the ADD?
  287      filled[hash] += a;       

  316      
  317:     // TODO assign amount or keep the ADD?
  318      filled[hash] += a;

  346  
  347:     // TODO assign amount or keep the ADD?
  348      filled[hash] += a;

  381      
  382:     // TODO assign amount or keep the ADD?
  383      filled[hash] += a;

  706    function deposit(uint8 p, address u, address c, uint256 a) internal returns (bool) {
  707:     // TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
  708:     if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
  709        return ICompound(c).mint(a) == 0;

  715        // a specified protocol contract whose address we have set
  716:       // TODO explain the Aave deposit args
  717        IAave(aaveAddr).deposit(u, a, address(this), 0);

  720        // Euler deposit is void.
  721:       // TODO explain the 0 (primary account)
  722        IEuler(c).deposit(0, a);

  739    function withdraw(uint8 p, address u, address c, uint256 a) internal returns (bool) {
  740:     // TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
  741:     if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
  742        return ICompound(c).redeemUnderlying(a) == 0;

  747        // Aave v2 docs state that withraw returns uint256
  748:       // TODO explain the withdraw args
  749        return IAave(aaveAddr).withdraw(u, a, address(this)) >= 0;

  751        // Euler withdraw is void
  752:       // TODO explain the 0
  753        IEuler(c).withdraw(0, a);

OPEN NOTES

File: Creator/Compounding.sol
L5:     import './Protocols.sol'; // NOTE: if size restrictions become extreme we can use ints (implicit enum)
        import './LibCompound.sol';
        import './LibFuse.sol';


L51:      // NOTE: the 1e26 const is a degree of precision to enforce on the return
              return IEulerToken(c).convertBalanceToUnderlying(1e26);
            } else {
          // NOTE: the 1e26 const is a degree of precision to enforce on the return
            return IErc4626(c).convertToAssets(1e26);

Using > 0 costs more gas than != 0 when used on a uint in a require() statement

File: Creator/VaultTracker.sol:
   53  
   54:     if (vlt.notional > 0) {
   55        uint256 yield;

   58        // otherwise, calculate marginal exchange rate between current and previous exchange rate.
   59:       if (maturityRate > 0) { // Calculate marginal interest
   60          yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;

   92      uint256 yield;
   93:     if (maturityRate > 0) { // Calculate marginal interest
   94        yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;

  122      uint256 yield;
  123:     if (maturityRate > 0) { // Calculate marginal interest
  124        yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;

  164      uint256 yield;
  165:     if (maturityRate > 0) { 
  166        // calculate marginal interest

  180      // transfer notional to address "t", calculate interest if necessary
  181:     if (to.notional > 0) {
  182        // if market has matured, calculate marginal interest between the maturity rate and previous position exchange rate
  183        // otherwise, calculate marginal exchange rate between current and previous exchange rate.
  184:       if (maturityRate > 0) { 
  185          // calculate marginal interest

  221        // otherwise, calculate marginal exchange rate between current and previous exchange rate.
  222:       if (maturityRate > 0) { 
  223          // calculate marginal interest

NatSpec is incomplete

Missing: @param p

File: Creator/VaultTracker.sol
L28:    /// @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) {

File: VaultTracker/VaultTracker.sol
L28:      /// @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) {