dforce-network / dToken

dForce yield token
MIT License
30 stars 10 forks source link

Duplicate handlers can be specified when setting handlers #15

Closed mgcolburn closed 4 years ago

mgcolburn commented 4 years ago

Severity: Low

Description

This is similar to #14.

Calls to setHandler do not explicitly check that input array does not contain duplicate Handler contract addresses. The system appears to handle this gracefully in most cases. However, if the default handler is specified multiple times, it may fail loudly. Regardless, it is clear from the implementation in addHandlers that duplicate handlers should not be allowed.

 function setHandlers( 
     address[] memory _handlers, 
     uint256[] memory _proportions 
 ) private { 
     require( 
         _handlers.length == _proportions.length && _handlers.length > 0, 
         "setHandlers: handlers & proportions should not have 0 or different lengths" 
     ); 

     // The 1st will act as the default handler. 
     defaultHandler = _handlers[0]; 

     uint256 _sum = 0; 
     for (uint256 i = 0; i < _handlers.length; i++) { 
         require( 
             _handlers[i] != address(0), 
             "setHandlers: handlerAddr contract address invalid" 
         ); 

         _sum = _sum.add(_proportions[i]); 

         handlers.push(_handlers[i]); 
         proportions[_handlers[i]] = _proportions[i]; 
         isHandlerActive[_handlers[i]] = true; 
     } 

     // The sum of proportions should be 1000000. 
     require( 
         _sum == totalProportion, 
         "the sum of proportions must be 1000000" 
     ); 
 } 

Recommendation

Add a check for duplicate Handlers to setHandler.

Donald-Nobel commented 4 years ago

Fixed.

joyious commented 4 years ago

Fixed in 0ec8168